From 10e1cbb359ef87d8563d34d0c5bac51b041ee5b2 Mon Sep 17 00:00:00 2001 From: Pavel Date: Mon, 27 Jul 2026 19:37:17 +0200 Subject: [PATCH] Copilot comments v1 --- application/api/user/agents/routes.py | 12 ++++---- application/logging.py | 4 +-- .../storage/db/repositories/stack_logs.py | 28 +++++++++-------- frontend/src/modals/AgentDetailsModal.tsx | 17 ++++++----- .../db/repositories/test_stack_logs.py | 30 +++++++++++++++++-- 5 files changed, 62 insertions(+), 29 deletions(-) diff --git a/application/api/user/agents/routes.py b/application/api/user/agents/routes.py index 65f4fb7f..b8c6a6f0 100644 --- a/application/api/user/agents/routes.py +++ b/application/api/user/agents/routes.py @@ -1359,11 +1359,13 @@ class RegenerateAgentKey(Resource): 500, ) - # Re-point key-string-only history to the new key so it is not - # orphaned. conversations/token analytics also match on - # agent_id, but stack_logs (webhook + system-error logs) has no - # agent_id column, and the 24h rate-limit window is keyed by the - # api-key string only. Runs in the same transaction as the swap. + # Re-point key-string history to the new key. conversations, + # token_usage and stack_logs (since 0026) all carry agent_id, so + # per-agent analytics survive rotation via agent_id regardless. + # These rewrites still run to (a) preserve the 24h rate-limit + # window, which is keyed by api_key only (token_usage), and (b) + # keep the api_key column consistent / re-attach any legacy rows + # whose agent_id is NULL. Runs in the same transaction as the swap. StackLogsRepository(conn).reassign_api_key( old_key=old_key, new_key=new_key ) diff --git a/application/logging.py b/application/logging.py index e5c7492c..5c03f2a2 100644 --- a/application/logging.py +++ b/application/logging.py @@ -5,7 +5,7 @@ import time import logging import uuid -from typing import Any, Callable, Dict, Generator, List +from typing import Any, Callable, Dict, Generator, List, Optional from application.core import log_context from application.storage.db.repositories.stack_logs import StackLogsRepository @@ -247,7 +247,7 @@ def _log_activity_to_db( query: str, stacks: List[Dict], level: str, - agent_id: str = None, + agent_id: Optional[str] = None, ) -> None: """Append a per-request activity log row to Postgres (``stack_logs``).""" try: diff --git a/application/storage/db/repositories/stack_logs.py b/application/storage/db/repositories/stack_logs.py index b09cdc5a..750376d6 100644 --- a/application/storage/db/repositories/stack_logs.py +++ b/application/storage/db/repositories/stack_logs.py @@ -10,6 +10,7 @@ Covers the single operation the legacy Mongo code performs: from __future__ import annotations import json +import uuid from datetime import datetime from typing import Optional @@ -57,14 +58,16 @@ class StackLogsRepository: stacks: Optional[list] = None, timestamp: Optional[datetime] = None, ) -> None: - # ``agent_id`` is a UUID column. Coerce anything not shaped like a UUID - # (36 chars with hyphens) to NULL so a stray/legacy id never breaks the - # activity-log write. Mirrors TokenUsageRepository.insert. + # ``agent_id`` is a UUID column. Parse it with ``uuid.UUID`` and coerce + # anything that isn't a valid UUID (e.g. a 24-hex legacy Mongo ObjectId) + # to NULL so a stray/legacy id never reaches ``CAST(... AS uuid)`` and + # breaks the activity-log write. agent_id_uuid: Optional[str] = None if agent_id: - s = str(agent_id) - if len(s) == 36 and "-" in s: - agent_id_uuid = s + try: + agent_id_uuid = str(uuid.UUID(str(agent_id))) + except (ValueError, AttributeError, TypeError): + agent_id_uuid = None self._conn.execute( text( """ @@ -98,17 +101,18 @@ class StackLogsRepository: def reassign_api_key(self, *, old_key: str, new_key: str) -> int: """Re-point historical rows from ``old_key`` to ``new_key``. - ``stack_logs`` has no ``agent_id`` column, so webhook / system-error - logs are matched by the api-key string alone (see the analytics - ``webhook_where`` / ``system_where`` clauses). When an agent's key is - rotated this rewrite keeps that history attached to the agent instead - of orphaning it. + Since migration 0026 ``stack_logs`` has an ``agent_id`` column and the + analytics ``webhook_where`` / ``system_where`` clauses match + ``agent_id`` first with ``api_key`` as a fallback, so most rows already + survive a key rotation via ``agent_id``. This rewrite still runs to keep + the ``api_key`` column consistent and to re-attach any rows whose + ``agent_id`` is NULL (e.g. a log written without agent context). Matched by ``api_key`` only — deliberately NOT scoped by ``user_id``: ``agents.key`` is globally unique, so a key maps to exactly one agent, and rows are stamped with the *caller's* user_id (which is not the owner for webhook / external-api-key traffic). Scoping by owner would - skip precisely the rows this migration exists to preserve. + skip precisely the rows this rewrite exists to preserve. Returns the number of rows updated. """ if not old_key or not new_key: diff --git a/frontend/src/modals/AgentDetailsModal.tsx b/frontend/src/modals/AgentDetailsModal.tsx index b519a9f5..b2bb8ee3 100644 --- a/frontend/src/modals/AgentDetailsModal.tsx +++ b/frontend/src/modals/AgentDetailsModal.tsx @@ -82,15 +82,18 @@ export default function AgentDetailsModal({ const handleRegenerateKey = async () => { setLoading('apiKey', true); - const response = await userService.regenerateAgentKey(agent.id ?? '', token); - if (!response.ok) { + try { + const response = await userService.regenerateAgentKey( + agent.id ?? '', + token, + ); + if (!response.ok) return; + const data = await response.json(); + setApiKey(data.key); + onKeyRegenerated?.(data.key); + } finally { setLoading('apiKey', false); - return; } - const data = await response.json(); - setApiKey(data.key); - onKeyRegenerated?.(data.key); - setLoading('apiKey', false); }; useEffect(() => { diff --git a/tests/storage/db/repositories/test_stack_logs.py b/tests/storage/db/repositories/test_stack_logs.py index 1e50ca52..798496d9 100644 --- a/tests/storage/db/repositories/test_stack_logs.py +++ b/tests/storage/db/repositories/test_stack_logs.py @@ -199,11 +199,35 @@ class TestInsert: assert dict(row._mapping)["stacks"][0]["data"]["result"] == "ok" +class TestAgentId: + def test_valid_agent_id_stored(self, pg_conn): + repo = _repo(pg_conn) + agent_id = "11111111-2222-3333-4444-555555555555" + repo.insert(activity_id="ai-valid", agent_id=agent_id) + row = pg_conn.execute( + text("SELECT agent_id FROM stack_logs WHERE activity_id = 'ai-valid'") + ).fetchone() + assert str(dict(row._mapping)["agent_id"]) == agent_id + + def test_invalid_agent_id_coerced_to_null(self, pg_conn): + # A 24-hex legacy Mongo ObjectId is not a valid UUID; it must be + # coerced to NULL rather than reaching CAST(... AS uuid) and killing + # the whole activity-log write. + repo = _repo(pg_conn) + repo.insert(activity_id="ai-legacy", agent_id="507f1f77bcf86cd799439011") + row = pg_conn.execute( + text("SELECT agent_id FROM stack_logs WHERE activity_id = 'ai-legacy'") + ).fetchone() + assert dict(row._mapping)["agent_id"] is None + + class TestReassignApiKey: def test_rewrites_matching_rows_regardless_of_user(self, pg_conn): - # stack_logs has no agent_id; on key rotation, rows must follow the - # key. Rows are stamped with the caller's user_id (not the owner), - # so the migration must NOT be scoped by user_id. + # On key rotation the rewrite is intentionally NOT scoped by user_id: + # agents.key is unique so a key maps to one agent, and rows are stamped + # with the caller's user_id (not the owner), so scoping by owner would + # skip caller-logged rows. (Rows also carry agent_id since 0026, but + # this rewrite still covers api_key consistency and NULL-agent_id rows.) repo = _repo(pg_conn) repo.insert(activity_id="ra-1", api_key="old-k", user_id="owner") repo.insert(activity_id="ra-2", api_key="old-k", user_id="a-caller")