mirror of
https://github.com/tiennm99/DocsGPT.git
synced 2026-10-15 18:15:33 +00:00
fix(admin): second review pass
Correctness - /api/remote never recorded source.created, so URL, GitHub and connector sources had a source.deleted with no matching creation. All three creation paths now go through one _audit_source_created helper. - The prompt-cache rate divided cached tokens by a whole bucket's prompt tokens. A bucket is a day and mixes calls whose provider reports a cache breakdown with calls whose provider does not, so filtering buckets in the client could not separate them and the rate was understated by however much traffic ran on a non-reporting provider. The denominator is now computed in SQL over the reporting rows. - The outcome pill matched values nothing writes. Guardrails emit triggered / not_evaluated and the device feed emits dispatched; the map had blocked / denied / allowed, so a guardrail that fired rendered neutral grey -- the one signal the merged feed exists to surface. Fixtures were seeding the fictional values, so the tests passed on it too. - Stream duration_ms timed the consumer. stream_token_usage is a generator, so start-to-exhaustion includes the agent loop's tool handling and the SSE client's pace; a slow browser recorded ~30s for a sub-second call. It now accumulates only the time spent inside next(). Safety - Activity filters failed open: an unknown facet or unparseable timestamp was dropped, and no filter means every row, so a typo widened an audit view and on the export streamed the full history. Both are now a 400. - The search term was interpolated into an ILIKE pattern, so "100%" matched everything and "q1_report" matched more than it should. Escaped. - 0034 set actor_id NOT NULL with no default. A previous-release process inserting mid-rollout would raise, and in admin/routes.py that insert shares the request transaction, so a role grant beside it would roll back too. Noise and dead code - The per-user panel is a security panel: data-plane events file under the actor, so an active account's routine deletes pushed a denied login out of the 20-row window. It now excludes them; the Activity tab shows everything. - device_audit_log had no created_at-leading index, so the merged feed sequentially scanned that branch every page (migration 0036). - conversation.deleted_all no longer records when nothing was deleted, and agent.updated no longer records an empty field list. - Dropped by_model from /admin/usage (no consumer; an extra aggregate per page load), the duplicate filter surface on AuthEventsRepository that nothing called, and the unreachable FLOW_LABELS.schedule entry. - Type hints on record_event's conn and the remaining unannotated helpers.
This commit is contained in:
1 parent
2ea9d66789
commit
25c82003d7
33 files changed
+635
-206
No files matched your search
@@ -129,19 +129,36 @@ class TestFeed:
|
||||
assert kwargs["until"] == datetime(2026, 2, 1, tzinfo=timezone.utc)
|
||||
assert kwargs["search"] == "handbook"
|
||||
|
||||
def test_unknown_facet_values_are_ignored_not_rejected(self, client):
|
||||
repo = _repo()
|
||||
with _admin(repo):
|
||||
resp = client.get("/api/admin/activity?feed=bogus&category=bogus")
|
||||
assert resp.status_code == 200
|
||||
kwargs = repo.list.call_args.kwargs
|
||||
assert kwargs["feeds"] is None and kwargs["categories"] is None
|
||||
def test_unknown_facet_is_refused_not_dropped(self, client):
|
||||
"""Dropping it would mean no filter, and no filter means everything.
|
||||
|
||||
def test_unparseable_time_is_ignored(self, client):
|
||||
A typo or a stale chip must never widen an audit view.
|
||||
"""
|
||||
repo = _repo()
|
||||
with _admin(repo):
|
||||
assert client.get("/api/admin/activity?since=nonsense").status_code == 200
|
||||
assert repo.list.call_args.kwargs["since"] is None
|
||||
resp = client.get("/api/admin/activity?category=bogus")
|
||||
assert resp.status_code == 400
|
||||
assert "bogus" in json.loads(resp.data)["message"]
|
||||
repo.list.assert_not_called()
|
||||
|
||||
def test_unknown_feed_is_refused(self, client):
|
||||
repo = _repo()
|
||||
with _admin(repo):
|
||||
assert client.get("/api/admin/activity?feed=bogus").status_code == 400
|
||||
repo.list.assert_not_called()
|
||||
|
||||
def test_unparseable_time_is_refused(self, client):
|
||||
"""Ignoring it would drop the bound and return the full history."""
|
||||
repo = _repo()
|
||||
with _admin(repo):
|
||||
assert client.get("/api/admin/activity?since=nonsense").status_code == 400
|
||||
repo.list.assert_not_called()
|
||||
|
||||
def test_a_valid_facet_still_passes_through(self, client):
|
||||
repo = _repo()
|
||||
with _admin(repo):
|
||||
assert client.get("/api/admin/activity?category=data").status_code == 200
|
||||
assert repo.list.call_args.kwargs["categories"] == ["data"]
|
||||
|
||||
def test_repeated_params_accumulate(self, client):
|
||||
repo = _repo()
|
||||
@@ -221,6 +238,15 @@ class TestExport:
|
||||
body = client.get("/api/admin/activity/export").get_data(as_text=True)
|
||||
assert body.strip().startswith("feed,id,event,category")
|
||||
|
||||
def test_export_refuses_an_unknown_facet(self, client):
|
||||
"""Failing open here would stream the whole history to the 100k cap."""
|
||||
repo = _repo()
|
||||
with _admin(repo):
|
||||
resp = client.get("/api/admin/activity/export?category=bogus")
|
||||
resp.get_data()
|
||||
assert resp.status_code == 400
|
||||
repo.iter_all.assert_not_called()
|
||||
|
||||
def test_rejects_an_unknown_format(self, client):
|
||||
with _admin(_repo()):
|
||||
assert (
|
||||
|
||||
@@ -238,12 +238,10 @@ class TestUsageAndAudit:
|
||||
"generated_tokens": 3,
|
||||
"cost": 0.25,
|
||||
"cached_tokens": None,
|
||||
"cache_eligible_prompt_tokens": 0,
|
||||
}
|
||||
]
|
||||
usage.sum_tokens_in_range.return_value = 8
|
||||
usage.tokens_by_model.return_value = [
|
||||
{"model_id": "gpt-x", "tokens": 8, "cost": 0.25}
|
||||
]
|
||||
stats = Mock()
|
||||
stats.top_token_users.return_value = [
|
||||
{"user_id": "a", "tokens": 8, "cost": 0.25}
|
||||
@@ -280,10 +278,20 @@ class TestUsageAndAudit:
|
||||
data = _body(client.get("/api/admin/usage"))
|
||||
assert data["total_cost"] == 0.25
|
||||
assert data["series"][0]["cost"] == 0.25
|
||||
assert data["by_model"][0]["model_id"] == "gpt-x"
|
||||
assert data["latency"]["p95_ms"] == 400
|
||||
assert data["top_users"][0]["cost"] == 0.25
|
||||
|
||||
def test_usage_does_not_run_an_aggregate_nothing_reads(self, client):
|
||||
"""Per-model spend is served by the per-user endpoint and by group_by."""
|
||||
usage, stats = self._usage_repos()
|
||||
with _admin(
|
||||
TokenUsageRepository=Mock(return_value=usage),
|
||||
AdminStatsRepository=Mock(return_value=stats),
|
||||
):
|
||||
data = _body(client.get("/api/admin/usage"))
|
||||
assert "by_model" not in data
|
||||
usage.tokens_by_model.assert_not_called()
|
||||
|
||||
def test_usage_group_by_reaches_the_repository(self, client):
|
||||
usage, stats = self._usage_repos()
|
||||
with _admin(
|
||||
|
||||
@@ -8,6 +8,7 @@ that the audit row is written with the acting user and the resource id.
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
from contextlib import ExitStack, contextmanager
|
||||
from unittest.mock import MagicMock, Mock, patch
|
||||
|
||||
@@ -79,6 +80,81 @@ class TestSourceAudit:
|
||||
assert recorded == []
|
||||
|
||||
|
||||
_UPLOAD = "docsgpt.api.user.sources.upload"
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestRemoteSourceAudit:
|
||||
"""``source.deleted`` is recorded for remote sources, so creation must be.
|
||||
|
||||
A trail showing a source deleted that was apparently never created is
|
||||
worse than no trail, and the docs state ``source.created`` is recorded.
|
||||
"""
|
||||
|
||||
@contextmanager
|
||||
def _remote_env(self):
|
||||
recorded: list[tuple] = []
|
||||
|
||||
def _record(_conn, event, *, actor, target=None, **metadata):
|
||||
recorded.append((event, actor, target, metadata))
|
||||
|
||||
@contextmanager
|
||||
def _conn():
|
||||
yield MagicMock()
|
||||
|
||||
task = Mock(id="task-9")
|
||||
with ExitStack() as stack:
|
||||
stack.enter_context(
|
||||
patch("docsgpt.app.handle_auth", return_value={"sub": "u1"})
|
||||
)
|
||||
stack.enter_context(
|
||||
patch("docsgpt.app.resolve_roles", return_value=["user"])
|
||||
)
|
||||
stack.enter_context(patch(f"{_UPLOAD}.db_session", _conn))
|
||||
stack.enter_context(patch(f"{_UPLOAD}.record_event", _record))
|
||||
stack.enter_context(
|
||||
patch(
|
||||
f"{_UPLOAD}.ingest_remote",
|
||||
Mock(apply_async=Mock(return_value=task)),
|
||||
)
|
||||
)
|
||||
yield recorded
|
||||
|
||||
def test_url_source_records_source_created(self, client):
|
||||
# The route reads ``request.form``; ``data`` is a JSON string.
|
||||
with self._remote_env() as recorded:
|
||||
resp = client.post(
|
||||
"/api/remote",
|
||||
data={
|
||||
"user": "u1",
|
||||
"source": "url",
|
||||
"name": "Handbook",
|
||||
"data": json.dumps({"url": "https://example.com"}),
|
||||
},
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
assert [row[0] for row in recorded] == ["source.created"]
|
||||
event, actor, target, metadata = recorded[0]
|
||||
assert (actor, target) == ("u1", None)
|
||||
assert metadata["name"] == "Handbook"
|
||||
assert metadata["type"] == "url"
|
||||
assert metadata["source_id"] == json.loads(resp.data)["source_id"]
|
||||
|
||||
def test_github_source_records_source_created(self, client):
|
||||
with self._remote_env() as recorded:
|
||||
resp = client.post(
|
||||
"/api/remote",
|
||||
data={
|
||||
"user": "u1",
|
||||
"source": "github",
|
||||
"name": "Repo",
|
||||
"data": json.dumps({"repo_url": "https://github.com/a/b"}),
|
||||
},
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
assert recorded[0][3]["type"] == "github"
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestAgentAudit:
|
||||
def test_delete_records_agent_deleted(self, client):
|
||||
|
||||
@@ -27,7 +27,7 @@ def seeded(pg_conn):
|
||||
"(user_id, api_key, stage, check_name, detector_type, action, "
|
||||
" outcome, matched_value, created_at) VALUES "
|
||||
"('u-guard', 'sk-secret', 'input', 'pii', 'regex', 'block', "
|
||||
" 'blocked', 'alex@example.com', :ts)"
|
||||
" 'triggered', 'alex@example.com', :ts)"
|
||||
),
|
||||
{"ts": now - timedelta(minutes=10)},
|
||||
)
|
||||
@@ -44,7 +44,7 @@ def seeded(pg_conn):
|
||||
"(device_id, user_id, invocation_id, action, command, "
|
||||
" approval_mode, decision, issued_at, created_at) VALUES "
|
||||
"('dev-1', 'u-dev', 'inv-1', 'run_command', 'ls -la', "
|
||||
" 'ask', 'allowed', :ts, :ts)"
|
||||
" 'ask', 'dispatched', :ts, :ts)"
|
||||
),
|
||||
{"ts": now - timedelta(minutes=5)},
|
||||
)
|
||||
@@ -84,9 +84,11 @@ class TestMergedFeed:
|
||||
}
|
||||
|
||||
def test_outcome_carries_the_side_journal_verdict(self, repo):
|
||||
# The values the writers actually produce: guardrails/runtime.py emits
|
||||
# triggered/not_evaluated, remote_device.py emits dispatched.
|
||||
rows = {row["feed"]: row["outcome"] for row in repo.list()}
|
||||
assert rows["device"] == "allowed"
|
||||
assert rows["guardrail"] == "blocked"
|
||||
assert rows["device"] == "dispatched"
|
||||
assert rows["guardrail"] == "triggered"
|
||||
assert rows["auth"] is None
|
||||
|
||||
|
||||
@@ -132,6 +134,12 @@ class TestFilters:
|
||||
assert repo.count(search="203.0.113") == 1
|
||||
assert repo.count(search="nope") == 0
|
||||
|
||||
def test_search_treats_wildcards_literally(self, repo):
|
||||
"""A search box takes a substring; '%' must not match everything."""
|
||||
assert repo.count(search="%") == 0
|
||||
# '_' would otherwise match any single character, e.g. "local".
|
||||
assert repo.count(search="l_cal") == 0
|
||||
|
||||
def test_unknown_feed_yields_nothing(self, repo):
|
||||
assert repo.list(feeds=["made-up"]) == []
|
||||
assert repo.count(feeds=["made-up"]) == 0
|
||||
|
||||
@@ -71,6 +71,31 @@ class TestBucketedCost:
|
||||
)
|
||||
assert series[0]["cached_tokens"] == 30
|
||||
|
||||
def test_cache_denominator_counts_only_reporting_rows(self, pg_conn, since):
|
||||
"""A hit rate needs the prompt tokens of the rows that reported.
|
||||
|
||||
Both rows land in the same bucket, so filtering buckets client-side
|
||||
cannot separate them: ``prompt_tokens`` there is the whole day. A rate
|
||||
built on it is understated by however much traffic ran on a provider
|
||||
that reports no cache breakdown.
|
||||
"""
|
||||
_insert(pg_conn, prompt_tokens=100, cached_tokens=50)
|
||||
_insert(pg_conn, prompt_tokens=100, cached_tokens=None)
|
||||
series = TokenUsageRepository(pg_conn).bucketed_totals(
|
||||
bucket_unit="day", timestamp_gte=since
|
||||
)
|
||||
assert series[0]["prompt_tokens"] == 200
|
||||
assert series[0]["cached_tokens"] == 50
|
||||
# 50 / 100 = 50%, not 50 / 200 = 25%.
|
||||
assert series[0]["cache_eligible_prompt_tokens"] == 100
|
||||
|
||||
def test_cache_denominator_is_zero_when_nothing_reported(self, pg_conn, since):
|
||||
_insert(pg_conn, prompt_tokens=100, cached_tokens=None)
|
||||
series = TokenUsageRepository(pg_conn).bucketed_totals(
|
||||
bucket_unit="day", timestamp_gte=since
|
||||
)
|
||||
assert series[0]["cache_eligible_prompt_tokens"] == 0
|
||||
|
||||
def test_grouping_keeps_cost_per_group(self, pg_conn, since):
|
||||
_insert(pg_conn, model_id="cheap", cost=0.1)
|
||||
_insert(pg_conn, model_id="pricey", cost=9.9)
|
||||
|
||||
@@ -45,41 +45,52 @@ class TestInsertAttribution:
|
||||
|
||||
|
||||
class TestFeedFilters:
|
||||
"""``/api/admin/audit`` is the single-journal feed; the merged one has its
|
||||
own repository, so the filter set here stays deliberately small."""
|
||||
|
||||
def _seed(self, repo):
|
||||
repo.insert("a", "oidc_login")
|
||||
repo.insert("b", "oidc_login_denied")
|
||||
repo.insert("b", "admin_user_deactivated", actor_id="admin-9", target_id="b")
|
||||
|
||||
def test_filter_by_actor(self, repo):
|
||||
self._seed(repo)
|
||||
rows = repo.list_all(actor_id="admin-9")
|
||||
assert [r["event"] for r in rows] == ["admin_user_deactivated"]
|
||||
assert repo.count_all(actor_id="admin-9") == 1
|
||||
|
||||
def test_filter_by_multiple_events(self, repo):
|
||||
self._seed(repo)
|
||||
rows = repo.list_all(events=["oidc_login", "oidc_login_denied"])
|
||||
assert {r["event"] for r in rows} == {"oidc_login", "oidc_login_denied"}
|
||||
assert repo.count_all(events=["oidc_login", "oidc_login_denied"]) == 2
|
||||
|
||||
def test_single_event_filter_still_supported(self, repo):
|
||||
def test_filter_by_event(self, repo):
|
||||
self._seed(repo)
|
||||
rows = repo.list_all(event="oidc_login")
|
||||
assert [r["event"] for r in rows] == ["oidc_login"]
|
||||
assert repo.count_all(event="oidc_login") == 1
|
||||
|
||||
def test_filter_by_until(self, repo):
|
||||
def test_filter_by_user_matches_subject_or_target(self, repo):
|
||||
self._seed(repo)
|
||||
assert repo.count_all(user_id="b") == 2
|
||||
|
||||
def test_filter_by_since(self, repo):
|
||||
self._seed(repo)
|
||||
future = datetime.now(timezone.utc) + timedelta(days=1)
|
||||
assert repo.count_all(since=future) == 0
|
||||
past = datetime.now(timezone.utc) - timedelta(days=1)
|
||||
assert repo.count_all(until=past) == 0
|
||||
assert repo.count_all(until=datetime.now(timezone.utc) + timedelta(days=1)) == 3
|
||||
assert repo.count_all(since=past) == 3
|
||||
|
||||
def test_search_matches_user_actor_and_metadata(self, repo):
|
||||
repo.insert("someone", "pat_created", metadata={"name": "ci-deploy-key"})
|
||||
assert repo.count_all(search="ci-deploy") == 1
|
||||
assert repo.count_all(search="someone") == 1
|
||||
assert repo.count_all(search="nothing-here") == 0
|
||||
|
||||
def test_event_names_catalogue(self, repo):
|
||||
def test_pagination(self, repo):
|
||||
self._seed(repo)
|
||||
names = repo.event_names()
|
||||
assert names == sorted(names)
|
||||
assert "oidc_login" in names and "admin_user_deactivated" in names
|
||||
assert len(repo.list_all(limit=2)) == 2
|
||||
assert len(repo.list_all(limit=2, offset=2)) == 1
|
||||
|
||||
|
||||
class TestPerUserPanel:
|
||||
"""The admin user drill-down is a security panel, not an activity log."""
|
||||
|
||||
def test_data_plane_events_can_be_excluded(self, repo):
|
||||
repo.insert("u1", "oidc_login_denied")
|
||||
for _ in range(5):
|
||||
repo.insert("u1", "conversation.deleted", target_id=None, actor_id="u1")
|
||||
rows = repo.list_recent(
|
||||
"u1", limit=3, exclude_events_like=("source.", "agent.", "conversation.")
|
||||
)
|
||||
# Without the exclusion the five deletes would fill the window and
|
||||
# push the denied login out of it.
|
||||
assert [r["event"] for r in rows] == ["oidc_login_denied"]
|
||||
|
||||
def test_without_exclusions_everything_is_returned(self, repo):
|
||||
repo.insert("u1", "oidc_login")
|
||||
repo.insert("u1", "conversation.deleted", target_id=None, actor_id="u1")
|
||||
assert len(repo.list_recent("u1")) == 2
|
||||
@@ -301,6 +301,7 @@ class TestBucketedTotals:
|
||||
"generated_tokens": 12,
|
||||
"cost": 0.75,
|
||||
"cached_tokens": None,
|
||||
"cache_eligible_prompt_tokens": 0,
|
||||
},
|
||||
{
|
||||
"bucket": "2026-04-11",
|
||||
@@ -308,6 +309,7 @@ class TestBucketedTotals:
|
||||
"generated_tokens": 1,
|
||||
"cost": 0.0,
|
||||
"cached_tokens": None,
|
||||
"cache_eligible_prompt_tokens": 0,
|
||||
},
|
||||
]
|
||||
|
||||
|
||||
@@ -126,6 +126,25 @@ class TestMigration0034RoundTrip:
|
||||
# Team events were always filed under the actor; they have no user target.
|
||||
assert rows["team.create"] == "actor-3|-"
|
||||
|
||||
def test_actor_id_has_a_default_for_rolling_deploys(self, pg_engine):
|
||||
"""A previous-release process must not 500 mid-rollout.
|
||||
|
||||
Its inserts name only the old columns; without a DEFAULT the NOT NULL
|
||||
raises, and in admin/routes.py that insert shares the request's
|
||||
transaction, so the role grant beside it would roll back too.
|
||||
"""
|
||||
with pg_engine.begin() as conn:
|
||||
conn.execute(
|
||||
text(
|
||||
"INSERT INTO auth_events (user_id, event, metadata) "
|
||||
"VALUES ('legacy', 'oidc_login', '{}'::jsonb)"
|
||||
)
|
||||
)
|
||||
actor = conn.execute(
|
||||
text("SELECT actor_id FROM auth_events WHERE user_id = 'legacy'")
|
||||
).scalar()
|
||||
assert actor == "unknown"
|
||||
|
||||
def test_backfill_scopes_quota_policy_targets(self, pg_engine):
|
||||
"""Only a user-scoped quota policy has a user target.
|
||||
|
||||
|
||||
@@ -83,6 +83,25 @@ class TestStreaming:
|
||||
assert persisted[0]["ttft_ms"] is None
|
||||
assert persisted[0]["duration_ms"] >= 0
|
||||
|
||||
def test_duration_excludes_consumer_backpressure(self, persisted):
|
||||
"""The clock must measure the provider, not a slow reader.
|
||||
|
||||
``stream_token_usage`` is a generator, so every yield suspends until
|
||||
the consumer comes back. Timing start-to-exhaustion would bill the
|
||||
agent loop's tool handling and the SSE client's pace to the model.
|
||||
"""
|
||||
import time as _time
|
||||
|
||||
@stream_token_usage
|
||||
def _stream(self, model, messages, stream, tools, **kwargs):
|
||||
yield "a"
|
||||
yield "b"
|
||||
|
||||
for _ in _stream(_LLM(), "m", [], True, None):
|
||||
# A consumer that takes far longer than the provider did.
|
||||
_time.sleep(0.05)
|
||||
assert persisted[0]["duration_ms"] < 50
|
||||
|
||||
def test_a_stream_cut_short_keeps_the_first_token_it_saw(self, persisted):
|
||||
@stream_token_usage
|
||||
def _stream(self, model, messages, stream, tools, **kwargs):
|
||||
|
||||
Reference in new issue
Block a user