fix(vector-sync): address round-2 review — one-shot backstop, helper, caps
- scanner: gate the consent backstop with a per-(user,doc_type) one-shot marker so a standing admin-disable doesn't re-enqueue idempotent deletes every scan tick; the marker clears when the type is re-enabled. Derive _TEXT_BACKSTOP_DOC_TYPES from INDEXED_DOC_TYPES so new indexed types are covered automatically - semantic: extract _consent_narrowed_doc_types so the search-side narrowing is unit-testable; add tests for restrict/intersect/disjoint/empty - purge route: cap doc_types length (abuse guard) -> 400 - tests: one-shot + re-enable backstop, too-many-doc_types 400 Deferred (noted on PR): per-document allowed_doc_types call is cache-hot; purge "last error wins" — both logged. SonarCloud broad-except hotspots are deliberate (noqa BLE001), reviewable in the UI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
477fb02b0a
commit
24b8000a71
@@ -32,6 +32,11 @@ from ..http import nextcloud_httpx_client
|
|||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
# Upper bound on doc_types per purge request. There are only a handful of real
|
||||||
|
# indexed types; this caps a hostile/buggy caller's fan-out of count+delete
|
||||||
|
# calls without constraining legitimate use.
|
||||||
|
_MAX_PURGE_DOC_TYPES = 64
|
||||||
|
|
||||||
|
|
||||||
async def purge_doc_types_route(request: Request) -> JSONResponse:
|
async def purge_doc_types_route(request: Request) -> JSONResponse:
|
||||||
"""POST /api/v1/vector-sync/purge — delete indexed vectors by doc type.
|
"""POST /api/v1/vector-sync/purge — delete indexed vectors by doc type.
|
||||||
@@ -83,6 +88,16 @@ async def purge_doc_types_route(request: Request) -> JSONResponse:
|
|||||||
doc_types = [d for d in raw if d]
|
doc_types = [d for d in raw if d]
|
||||||
if not doc_types:
|
if not doc_types:
|
||||||
return JSONResponse({"purged": {}})
|
return JSONResponse({"purged": {}})
|
||||||
|
# Bound the batch: there are only a handful of real indexed types, so a huge
|
||||||
|
# list is abuse — cap it rather than fan out unbounded count+delete calls.
|
||||||
|
if len(doc_types) > _MAX_PURGE_DOC_TYPES:
|
||||||
|
return JSONResponse(
|
||||||
|
{
|
||||||
|
"error": "Bad request",
|
||||||
|
"message": f"doc_types exceeds the maximum of {_MAX_PURGE_DOC_TYPES}",
|
||||||
|
},
|
||||||
|
status_code=400,
|
||||||
|
)
|
||||||
|
|
||||||
try:
|
try:
|
||||||
username, app_password = await get_basic_auth_for_user(user_id)
|
username, app_password = await get_basic_auth_for_user(user_id)
|
||||||
|
|||||||
@@ -57,6 +57,23 @@ logger = logging.getLogger(__name__)
|
|||||||
_USAGE_METADATA_MAX_DOC_TYPES = 16
|
_USAGE_METADATA_MAX_DOC_TYPES = 16
|
||||||
|
|
||||||
|
|
||||||
|
def _consent_narrowed_doc_types(
|
||||||
|
doc_types: list[str] | None, allowed: frozenset[str]
|
||||||
|
) -> list[str]:
|
||||||
|
"""Apply the admin allow-set to a requested ``doc_types`` filter.
|
||||||
|
|
||||||
|
Caller has already established ``allowed is not None`` (a concrete allow-set;
|
||||||
|
``None`` means "no restriction" and is handled by skipping this call). When
|
||||||
|
no explicit ``doc_types`` are requested, restrict to the full allow-set;
|
||||||
|
otherwise intersect (preserving the caller's order). An empty result means
|
||||||
|
nothing the caller asked for is admin-approved — the caller short-circuits
|
||||||
|
to an empty response rather than falling through to an all-types search.
|
||||||
|
"""
|
||||||
|
if doc_types is None:
|
||||||
|
return sorted(allowed)
|
||||||
|
return [dt for dt in doc_types if dt in allowed]
|
||||||
|
|
||||||
|
|
||||||
async def record_search_usage(
|
async def record_search_usage(
|
||||||
*,
|
*,
|
||||||
enabled: bool,
|
enabled: bool,
|
||||||
@@ -309,10 +326,7 @@ def configure_semantic_tools(mcp: FastMCP):
|
|||||||
# means the admin disabled every source.
|
# means the admin disabled every source.
|
||||||
allowed = await allowed_doc_types(client, username)
|
allowed = await allowed_doc_types(client, username)
|
||||||
if allowed is not None:
|
if allowed is not None:
|
||||||
if doc_types is None:
|
doc_types = _consent_narrowed_doc_types(doc_types, allowed)
|
||||||
doc_types = sorted(allowed)
|
|
||||||
else:
|
|
||||||
doc_types = [dt for dt in doc_types if dt in allowed]
|
|
||||||
if not doc_types:
|
if not doc_types:
|
||||||
logger.info(
|
logger.info(
|
||||||
"Semantic search short-circuited for user %s: no requested "
|
"Semantic search short-circuited for user %s: no requested "
|
||||||
|
|||||||
@@ -282,9 +282,16 @@ def _app_enabled(app_id: str, enabled_apps: set[str] | None) -> bool:
|
|||||||
|
|
||||||
# Text doc types whose deletion-tracking lives *inside* their scan_* function,
|
# Text doc types whose deletion-tracking lives *inside* their scan_* function,
|
||||||
# so skipping that function (when admin-disabled) leaves indexed points with no
|
# so skipping that function (when admin-disabled) leaves indexed points with no
|
||||||
# grace-period backstop. ``file`` is intentionally excluded: its scan path
|
# grace-period backstop. Derived from INDEXED_DOC_TYPES so a newly-indexed type
|
||||||
# empties discovery and lets the existing reconcile loop purge on disable.
|
# automatically gets the backstop. ``file`` is excluded: its scan path empties
|
||||||
_TEXT_BACKSTOP_DOC_TYPES: tuple[str, ...] = ("note", "news_item", "deck_card")
|
# discovery and lets the existing reconcile loop purge on disable.
|
||||||
|
_TEXT_BACKSTOP_DOC_TYPES: tuple[str, ...] = tuple(sorted(INDEXED_DOC_TYPES - {"file"}))
|
||||||
|
|
||||||
|
# Per-process record of (user_id, doc_type) whose consent backstop deletes have
|
||||||
|
# already been enqueued, so a *standing* admin-disable doesn't re-flood the
|
||||||
|
# processor with idempotent deletes on every scan tick. An entry is cleared once
|
||||||
|
# the type is allowed again, so a later re-disable re-triggers the backstop.
|
||||||
|
_consent_backstop_done: set[tuple[str, str]] = set()
|
||||||
|
|
||||||
|
|
||||||
async def _enqueue_deletes_for_disabled_types(
|
async def _enqueue_deletes_for_disabled_types(
|
||||||
@@ -303,7 +310,18 @@ async def _enqueue_deletes_for_disabled_types(
|
|||||||
if allowed is None:
|
if allowed is None:
|
||||||
return 0
|
return 0
|
||||||
|
|
||||||
disabled = [dt for dt in _TEXT_BACKSTOP_DOC_TYPES if dt not in allowed]
|
# Re-enabled types: clear their one-shot marker so a later re-disable
|
||||||
|
# re-triggers the backstop.
|
||||||
|
for doc_type in _TEXT_BACKSTOP_DOC_TYPES:
|
||||||
|
if doc_type in allowed:
|
||||||
|
_consent_backstop_done.discard((user_id, doc_type))
|
||||||
|
|
||||||
|
# Disabled types not yet backstopped this episode.
|
||||||
|
disabled = [
|
||||||
|
dt
|
||||||
|
for dt in _TEXT_BACKSTOP_DOC_TYPES
|
||||||
|
if dt not in allowed and (user_id, dt) not in _consent_backstop_done
|
||||||
|
]
|
||||||
if not disabled:
|
if not disabled:
|
||||||
return 0
|
return 0
|
||||||
|
|
||||||
@@ -346,6 +364,9 @@ async def _enqueue_deletes_for_disabled_types(
|
|||||||
)
|
)
|
||||||
)
|
)
|
||||||
queued += 1
|
queued += 1
|
||||||
|
# Mark this (user, doc_type) backstopped for the current disable episode
|
||||||
|
# so subsequent scans don't re-enqueue the same idempotent deletes.
|
||||||
|
_consent_backstop_done.add((user_id, doc_type))
|
||||||
return queued
|
return queued
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,37 @@
|
|||||||
|
"""Unit tests for the search-side consent narrowing in nc_semantic_search.
|
||||||
|
|
||||||
|
The narrowing logic (intersect requested doc_types with the admin allow-set,
|
||||||
|
or restrict to the allow-set when none requested) is extracted into
|
||||||
|
``_consent_narrowed_doc_types`` so it can be tested without exercising the full
|
||||||
|
search path. ``allowed is None`` (no restriction / fail-open) is handled by the
|
||||||
|
caller skipping this helper entirely.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from nextcloud_mcp_server.server.semantic import _consent_narrowed_doc_types
|
||||||
|
|
||||||
|
|
||||||
|
def test_none_request_restricts_to_allow_set():
|
||||||
|
# No explicit doc_types -> search exactly the allowed set (sorted).
|
||||||
|
assert _consent_narrowed_doc_types(None, frozenset({"file", "note"})) == [
|
||||||
|
"file",
|
||||||
|
"note",
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
|
def test_request_intersected_with_allow_set_preserves_order():
|
||||||
|
result = _consent_narrowed_doc_types(
|
||||||
|
["deck_card", "note", "file"], frozenset({"note", "file"})
|
||||||
|
)
|
||||||
|
assert result == ["note", "file"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_disjoint_request_yields_empty():
|
||||||
|
# Caller short-circuits to an empty response on [].
|
||||||
|
assert _consent_narrowed_doc_types(["deck_card"], frozenset({"note"})) == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_empty_allow_set_blocks_all():
|
||||||
|
assert _consent_narrowed_doc_types(None, frozenset()) == []
|
||||||
|
assert _consent_narrowed_doc_types(["note"], frozenset()) == []
|
||||||
@@ -151,6 +151,20 @@ def test_bad_request_when_body_not_object(mocker):
|
|||||||
purge.assert_not_called()
|
purge.assert_not_called()
|
||||||
|
|
||||||
|
|
||||||
|
def test_bad_request_when_too_many_doc_types(mocker):
|
||||||
|
_patch_token(mocker)
|
||||||
|
purge = _patch_purge(mocker)
|
||||||
|
|
||||||
|
client = TestClient(_build_app())
|
||||||
|
resp = client.post(
|
||||||
|
"/api/v1/vector-sync/purge",
|
||||||
|
json={"doc_types": [f"t{i}" for i in range(65)]},
|
||||||
|
)
|
||||||
|
|
||||||
|
assert resp.status_code == 400
|
||||||
|
purge.assert_not_called()
|
||||||
|
|
||||||
|
|
||||||
def test_provisioning_required_returns_428(mocker):
|
def test_provisioning_required_returns_428(mocker):
|
||||||
_patch_token(mocker, "admin")
|
_patch_token(mocker, "admin")
|
||||||
mocker.patch(
|
mocker.patch(
|
||||||
|
|||||||
@@ -12,11 +12,21 @@ from types import SimpleNamespace
|
|||||||
from typing import cast
|
from typing import cast
|
||||||
from unittest.mock import AsyncMock
|
from unittest.mock import AsyncMock
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
from nextcloud_mcp_server.vector import scanner as scanner_module
|
from nextcloud_mcp_server.vector import scanner as scanner_module
|
||||||
from nextcloud_mcp_server.vector.queue.ports import TaskProducer
|
from nextcloud_mcp_server.vector.queue.ports import TaskProducer
|
||||||
from nextcloud_mcp_server.vector.scanner import _enqueue_deletes_for_disabled_types
|
from nextcloud_mcp_server.vector.scanner import _enqueue_deletes_for_disabled_types
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture(autouse=True)
|
||||||
|
def _clear_backstop_state():
|
||||||
|
"""The one-shot guard is module-level; reset it between tests."""
|
||||||
|
scanner_module._consent_backstop_done.clear()
|
||||||
|
yield
|
||||||
|
scanner_module._consent_backstop_done.clear()
|
||||||
|
|
||||||
|
|
||||||
def _producer(send: AsyncMock) -> TaskProducer:
|
def _producer(send: AsyncMock) -> TaskProducer:
|
||||||
"""A minimal stand-in for the TaskProducer protocol (only ``send`` is used)."""
|
"""A minimal stand-in for the TaskProducer protocol (only ``send`` is used)."""
|
||||||
return cast(TaskProducer, SimpleNamespace(send=send))
|
return cast(TaskProducer, SimpleNamespace(send=send))
|
||||||
@@ -79,3 +89,42 @@ async def test_noop_when_all_text_types_allowed(monkeypatch):
|
|||||||
)
|
)
|
||||||
assert queued == 0
|
assert queued == 0
|
||||||
send.assert_not_called()
|
send.assert_not_called()
|
||||||
|
|
||||||
|
|
||||||
|
async def test_one_shot_does_not_reflood_on_subsequent_scans(monkeypatch):
|
||||||
|
_patch_qdrant(monkeypatch, {"note": ["n1", "n2"]})
|
||||||
|
send = AsyncMock()
|
||||||
|
allowed = frozenset({"file"}) # note disabled
|
||||||
|
|
||||||
|
first = await _enqueue_deletes_for_disabled_types(
|
||||||
|
"alice", _producer(send), allowed, 1
|
||||||
|
)
|
||||||
|
second = await _enqueue_deletes_for_disabled_types(
|
||||||
|
"alice", _producer(send), allowed, 2
|
||||||
|
)
|
||||||
|
|
||||||
|
assert first == 2
|
||||||
|
# Standing disable: the next scan must not re-enqueue the same deletes.
|
||||||
|
assert second == 0
|
||||||
|
|
||||||
|
|
||||||
|
async def test_re_enable_then_disable_retriggers_backstop(monkeypatch):
|
||||||
|
_patch_qdrant(monkeypatch, {"note": ["n1"]})
|
||||||
|
send = AsyncMock()
|
||||||
|
disabled = frozenset({"file"})
|
||||||
|
enabled = frozenset({"file", "note"})
|
||||||
|
|
||||||
|
assert (
|
||||||
|
await _enqueue_deletes_for_disabled_types("alice", _producer(send), disabled, 1)
|
||||||
|
== 1
|
||||||
|
)
|
||||||
|
# Re-enabled: clears the one-shot marker.
|
||||||
|
assert (
|
||||||
|
await _enqueue_deletes_for_disabled_types("alice", _producer(send), enabled, 2)
|
||||||
|
== 0
|
||||||
|
)
|
||||||
|
# Disabled again: backstop fires once more.
|
||||||
|
assert (
|
||||||
|
await _enqueue_deletes_for_disabled_types("alice", _producer(send), disabled, 3)
|
||||||
|
== 1
|
||||||
|
)
|
||||||
|
|||||||
Reference in New Issue
Block a user