076ec91981cf3ae60baf7ea6a4236e1ee879f19c
599
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Chris CoutinhoandClaude Opus 4.8
|
29500bea7e |
fix: PR #813 review — cap unified_search multi-type pool; document deck self-only
🟡 Performance: unified_search's _execute sorted but did not cap the merged multi-doc_type pool, so N doc_types each fetched at search_limit sent N*search_limit candidates into verify-on-read (one Nextcloud round-trip each). Cap to search_limit*2 after the sort, matching vector_search, nc_semantic_search and the viz_routes pattern — bounding verification cost to O(2*search_limit) regardless of how many doc_types are requested. 🟡 Consistency: _get_deck_metadata_from_qdrant is the one internal Qdrant lookup that uses a raw user_id filter instead of build_ownership_filter. This is not a bug — deck cards are a documented cross-user gap (the Deck API is per-user, so cross-user context can't be fetched with the caller's credentials) — but the inconsistency was unexplained. Added a comment documenting the deliberate self-only scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.8
|
063bd06ba6 |
fix: cache app-password storage to avoid per-request Alembic upgrade race
get_user_client_basic_auth built a fresh RefreshTokenStorage and ran storage.initialize() — a full Alembic `upgrade` in a worker thread — on every call. Once the /api/v1 search endpoints (unified_search, vector_search) and the chunk-context endpoint were wired to use it, concurrent requests ran concurrent Alembic upgrades, which race on Alembic's non-thread-safe module-global EnvironmentContext proxy and intermittently raise `KeyError: 'script'` → HTTP 500 (seen on multi-user-basic/nc31; nc32 got lucky). Cache one process-wide, already-initialized storage instance behind a lazily created anyio.Lock so the one-time migration runs exactly once and never races. Callers passing an explicit `storage` are unaffected. Also removes a redundant per-request migration from the search hot path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.8
|
350358b802 |
fix: PR #813 review — shared-file context in MCP tool path + viz over-fetch cap
🟡 Important: nc_semantic_search's include_context branch did not forward accessible_owners to get_chunk_with_context, so context expansion for shared files stayed self-only, found nothing in Qdrant, and silently fell back to the plain excerpt. Forward accessible_owners (the per-file file_accessible_by_id gate still enforces access). 🟡 Performance: auth/viz_routes.py's multi-doc_type branch sorted but did not cap the candidate pool before verify-on-read, so N doc_types × limit*2 went into verification (N× the Nextcloud round-trips). Cap to limit*2 after the sort, matching server/semantic.py and the cross-app branch. Also clear the SonarCloud gate (new_duplicated_lines_density 5.1% > 3%) the ACL wiring introduced: extract the duplicated /api/v1 client-resolution + owner-expansion + verify-on-read block from unified_search/vector_search into a shared _search_with_acl helper, define a constant for the repeated "Nextcloud host not configured" literal (S1192), and reword the access_filter move_to_end comment so it isn't misread as commented-out code (S125) while adding the other-owner count to its debug log (review nits). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.8
|
8deb48e6fa |
fix: address PR #813 review round 4 (log leak, cross-user chunk ctx, algo, overlap)
1. Don't log unverified result titles: both search algorithms logged top-5 titles at DEBUG before verify-on-read; with owner-level share expansion the unverified set can contain other users' docs. Algorithms now log a count only; the verifying callers (server/semantic, viz_routes, api/visualization) log verified titles after verify-on-read. 2. Cross-user FILE chunk context: get_chunk_with_context + the Qdrant chunk helpers now take accessible_owners and use build_ownership_filter. For files the expanded scope is honoured only after a per-file file_accessible_by_id check (accessible_owners is owner-level, so the gate prevents a one-file share recipient from reading any of the owner's cached chunks). note/deck/ news stay self-only (per-user APIs) — a documented gap. Both chunk endpoints pass accessible_owners. 3. Algorithm usage: SemanticSearchAlgorithm is not dead (it backs the dense-only option on the viz/API surfaces); added a clarifying comment in server/ semantic.py. Additionally wired accessible_owners + verify-on-read into the /api/v1 search routes (unified_search, vector_search) so the astrolabe surface is ACL-aware too — degrading gracefully to self-only/unverified for non-provisioned callers instead of 401. 4. Overlapping conditions: build_ownership_filter no longer lists self in the owner_id MatchAny branch (self is already covered by the user_id branch); the owner_id branch carries only the OTHER owners. Tests: build_ownership_filter dedup + chunk-bbox filter-shape updates; new ACL-aware get_indexed_doc_types, cached-chunk lookup, and end-to-end cross-user file chunk-context (recipient gets the chunk, non-recipient denied) tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.8
|
423d0a1758 |
fix: address PR #813 latest review (ACL-aware doc-type discovery, robustness)
- get_indexed_doc_types: add optional accessible_owners param and reuse
build_ownership_filter so cross-user doc-type discovery matches the real
search scope (was self-only / ACL-blind); docstring documents the self-only
default. Covered by test_get_indexed_doc_types_is_acl_aware.
- access_filter: build_ownership_filter now omits the owner_id branch entirely
for an empty owner set instead of relying on undocumented MatchAny(any=[])
semantics; updated the empty-list unit test accordingly.
- access_filter: make the uid_owner/owner share-owner extraction explicit
("absent, not empty") to avoid skipping on a falsy-but-present field.
- access_filter: add an operator note that pre-owner_id points need a re-index
to surface to share recipients (ACL search is a no-op for legacy data).
- verification/webdav: lock the file_accessible_by_id(scope="") contract with a
targeted multi-user test (owner + recipient True, non-recipient False).
- viz_routes: comment that verify-on-read eviction runs inline by design (no
lifespan task group available on the Starlette route).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude Opus 4.8
|
cafbfd15a9 |
fix: address PR #813 review (owner_id index, cache bound, explicit param)
- vector/qdrant_client.py: add owner_id to _PAYLOAD_INDEX_FIELDS (BLOCKING). Every search applies MatchAny(key="owner_id", ...); without a keyword index Qdrant full-scans the collection and may 400 on Qdrant Cloud strict mode. _ensure_payload_indexes is idempotent so existing collections migrate at startup. - search/access_filter.py: bound the process-global _owners_cache with an LRU cap (was one unbounded entry per active user, never evicted); document the owner-level over-fetch limitation (a prolific sharer floods the recall buffer with ghost candidates that verify-on-read drops, with no second Qdrant pass) as a TODO toward per-file filtering. - search/algorithms.py + semantic.py + bm25_hybrid.py: promote accessible_owners from **kwargs to an explicit keyword-only parameter on the SearchAlgorithm ABC and both implementations, so a misspelled keyword is a type error rather than a silent fall back to self-only scope. - search/verification.py: document that _verify_files now verifies by global file id (WebDAV SEARCH), not by path. - tests/unit/search/test_access_filter.py: add cache-hit, TTL-expiry, failure-not-cached, and LRU-bound tests. Bumps the astrolabe submodule with the matching #89 review fixes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.8
|
b1fac2d7a8 |
fix(search): address PR #813 review (viz verify-on-read, owners cache, docs)
- viz_routes: run verify_search_results before returning results. After the accessible_owners expansion the viz can surface OTHER users' shared docs, so it must drop ones the caller can no longer access (revoked share) — same as the nc_semantic_search tool path. (Blocking review item.) - access_filter: cache list_accessible_owners per user for 30s to keep the OCS shares round-trip off the search hot path (failures aren't cached); document the single-page OCS limitation; add a clear_accessible_owners_cache() test helper. Comment the empty-accessible_owners MatchAny([]) edge case. - verification: comment why cross-user eviction is a deliberate no-op (eviction is scoped to the querying user's id, so a recipient's revoked access never deletes the owner's points; the recipient self-heals via accessible_owners). - algorithms: declare SearchResult.original_score (set by the viz route) so the now-precisely-typed result list type-checks. - tests: cross-user eviction-no-op safety test; autouse owners-cache reset in the access_filter + shared-search tests; replace async-no-await qdrant fakes with AsyncMock (clears SonarCloud S7503). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.8
|
d9a716080a |
fix(auth): make provision/revoke consistent with the app-password store
The OAuth provisioning tools (check_provisioning_status, revoke_nextcloud_ access) only consulted the refresh-token store + Astrolabe status, ignoring the app_passwords store that Login Flow v2 (nc_auth_provision_access) and the management API write to — the same store require_provisioning / get_client use to grant tool access. Result: status reported "not provisioned" while tools worked, and revoke said "nothing to revoke" while the credential persisted. - _get_provisioning_status: also check storage.get_app_password_with_scopes, reporting is_provisioned with credential_type=app_password, flow_type=login_flow_v2. - _revoke_nextcloud_access: when the credential is an app password, delete it from storage + invalidate the scope cache (no IdP token to revoke); refresh-token revocation via the Token Broker is unchanged. - tests/unit/test_oauth_tools_app_password_provisioning.py: cover status + revoke for the app-password path. - bump astrolabe submodule (deprovision MCP on disable); fix a stale assertion in the migrated bg-sync test (one-click flow has no separate app-password generation step). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.8
|
ae54956f27 |
fix(auth): login-flow provisioning — public login_url + session app passwords
Two fixes surfaced while testing Login Flow v2 provisioning behind a split internal/external host (Docker: server↔Nextcloud over http://app, browser over http://localhost:8080): 1. login_url pointed at the internal host. Nextcloud builds the login URL from the request host, so the browser-facing URL came back as http://app/login/v2/flow/... — unreachable from the user's browser. The poll endpoint was already rewritten to the internal host (correct, the server polls it); now LoginFlowV2Client also rewrites the login_url origin to settings.nextcloud_public_issuer_url when set (passed at all 5 construction sites). When unset, behaviour is unchanged. 2. The app-password format guard rejected raw session tokens. core/ getapppassword returns a long alphanumeric token, not the dashed 25-char Security-settings format, so the dashed-only regex 400'd the one-click opt-in handoff. Relax APP_PASSWORD_PATTERN to `^[a-zA-Z0-9-]{20,256}$`; the authoritative validation is still the BasicAuth check against Nextcloud. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.8
|
bf35200bab |
fix(search): verify shared files by global file id (ACL-aware)
The ACL-aware vector filter (PR #813) expands a user's search to documents whose owner shared them, but verify-on-read still re-checked each file by PATH under the *searching* user's WebDAV root. Nextcloud mounts received shares at the recipient's root by basename, so a nested shared file (e.g. owner's /docs/report.pdf) 404s for the recipient and was silently dropped — defeating the filter for everything but root-level files. Verify files by their global Nextcloud file id instead (the file doc_id IS that id): WebDAVClient.get_file_info_by_id was insufficient (the dav/meta endpoint only resolves the user's own storage, not shares), so add WebDAVClient.file_accessible_by_id which runs a WebDAV SEARCH over the user's whole tree (incl. mounted shares) filtered on oc:fileid. Empirically this resolves owned, directly-shared, and folder-shared files; an empty result is a definitive drop, transport errors are kept as transient. - search/verification.py: _verify_files now checks file_accessible_by_id. - client/webdav.py: add file_accessible_by_id (SEARCH by fileid). - tests/integration/test_acl_owner_filter.py: filter matrix vs real Qdrant. - tests/integration/test_acl_shared_search.py: real-Nextcloud share -> search. - tests/integration/test_verify_on_read.py: nested shared file kept for the recipient; unshared file dropped. - tests/unit/search/test_verification.py: id-based verifier semantics. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
37db82613d |
feat(search): ACL-aware vector filter via Nextcloud Shares lookup
The vector index has always been strictly per-user: every Qdrant payload
carries a `user_id` and the search filter is `user_id == querying_user`.
A file Alice indexed cannot be discovered by Bob even if she has shared
it with him — Bob would have to re-index it under his own user_id to
make it searchable, which means duplicate index entries for every share
recipient.
Switch to ownership-with-ACL-expansion:
- New `nextcloud_mcp_server.search.access_filter` module:
- `list_accessible_owners(sharing_client, user_id)` calls the OCS
Sharing API (`shared_with_me=true`) and returns
`{user_id} ∪ {uid_owner of each share}`. Fails open to `[user_id]`
so a misbehaving Sharing API doesn't black-hole search.
- `build_ownership_filter(user_id, accessible_owners)` returns a
Qdrant `Filter` whose `should` branch matches either the new
`owner_id IN accessible_owners` field or the legacy `user_id` field.
The legacy branch keeps points indexed before this change reachable
without a migration backfill.
- Indexer payload (`vector/processor.py`) now writes `owner_id` alongside
`user_id`. `DocumentTask` gains an optional `owner_id` field; today the
scanner always runs as the owner so the processor falls back to
`user_id`, but the field is plumbed so a future shared-with-me crawler
can set the true owner without reshaping the payload contract.
- `SemanticSearchAlgorithm.search` and `BM25HybridSearchAlgorithm.search`
accept `accessible_owners` via kwargs and use the new ownership filter.
Default behaviour with no kwarg is unchanged (self-only).
- Both user-facing callers — the MCP tool path (`server/semantic.py`) and
the visualization Starlette route (`auth/viz_routes.py`) — compute
`accessible_owners` from the authenticated Nextcloud client before
invoking the search algorithm. Eviction, scanner deletion, placeholder,
and chunk-context paths intentionally keep the legacy `user_id`
semantics (those are "operations on a specific user's records", not
cross-user reads).
- 10 new unit tests in `tests/unit/search/test_access_filter.py` cover
self-only default, owner expansion, dedup, fallback fields, OCS
failure, and the legacy `should`-branch shape.
Pairs with cbcoutinho/astrolabe#89 — together they let an Astrolabe user
find content owners have shared with them without going through any
re-authorization flow or re-indexing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandGitHub
|
baf4d4d225 |
Merge pull request #809 from cbcoutinho/feat/deck-webhook-presets
feat(webhooks): add Deck card sync preset with vector indexing |
||
|
Chris CoutinhoandClaude Opus 4.7
|
c03223a41c |
fix(vector-sync): use resolved collection name in orphan sweep
The startup sweep was reading settings.qdrant_collection (the raw config
value, default "nextcloud_content") instead of settings.get_collection_name(),
which is what every other vector-sync operation uses. When QDRANT_COLLECTION
is not overridden, get_collection_name() auto-generates a
{deployment-id}-{model-name} name; the sweep was targeting a non-existent
collection and silently returning (0, 0).
Also adds the AsyncQdrantClient type annotation that was missing on
sweep_orphan_placeholders, and renames its parameter from collection_name
to collection to make it clear the value must be the resolved name.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
a5cbe91b29 |
fix(vector-sync): sweep placeholder orphans at Pod startup (#101)
When the per-tenant nextcloud-mcp-server Pod OOMKills mid-batch, the in-memory anyio processor queue is lost but the placeholder Qdrant points (is_placeholder=true, status=pending) survive. The next Pod's scanner re-runs, sees the existing placeholders, applies the 5 × VECTOR_SYNC_SCAN_INTERVAL staleness gate (~5h with the deployed 1h scan interval), and skips them. Result: 0 documents indexed for the duration of the gate after every restart. Stamps a process-level instance_id (UUID per Pod-process) onto every placeholder write. A new sweep_orphan_placeholders helper, called once from starlette_lifespan after the Qdrant client is initialised and before the scanner / user-manager spawns, scrolls the collection and deletes any placeholder whose instance_id doesn't match the current Pod's (including placeholders with no instance_id field — back-compat for pre-fix Pod versions). The scanner's next cycle naturally re-creates fresh placeholders and queues work normally; no DocumentTask reconstruction needed. Sweep is one-shot at startup, not periodic — the existing staleness gate still covers same-Pod recovery, and the cross-Pod-restart gap was the only failure mode. Failure is non-fatal (logged via vector_sync.orphan_sweep_failed) so a transient Qdrant hiccup at boot doesn't prevent the scanner from running. Both lifespan branches (single-user BasicAuth, OAuth / multi-user BasicAuth) call the sweep via a module-local helper. A new VECTOR_SYNC_ORPHAN_SWEEP_ENABLED setting (default True) provides an escape hatch. Closes Deck #101. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
688a03f00a |
fix(contacts): surface ORG/TITLE/NOTE/URL/CATEGORIES/PHOTO on read (refs #716)
PR #719 fixed the contact-create path so all documented fields persist to the vCard, but the read path (list/search via MCP) still returned ``organization: null`` / ``note: null`` / ``title: null`` because pythonvCard4 has no typed parser for ORG/TITLE — they land in ``Contact.custom`` — and the server-side mapper never read ``note``/``urls``/``categories``/``photo`` even when present. Reads now surface what the write side persisted: - ``client/contacts.py``: new ``_first_custom`` helper pulls raw values from ``Contact.custom`` for ORG/TITLE/unencoded PHOTO. ``list_contacts`` extends its per-contact dict with org/title/note/url/categories/photo. - ``server/contacts.py``: ``_raw_contact_to_model`` maps the new keys onto ``Contact.organization`` / ``.title`` / ``.note`` / ``.urls`` / ``.categories`` / ``.photo``. URL accepts both list and plain-string shapes; categories accepts comma-separated strings for forward-compat. Coverage: - Unit: ``TestFirstCustom`` (five cases incl. bare-string library shape) and three new ``_raw_contact_to_model`` cases covering the full field set, plain-string URL, and comma-string categories. - Integration: ``test_mcp_contacts_workflow`` now decodes the ``nc_contacts_search_contacts`` response and asserts ``organization`` / ``note`` round-trip — direct regression coverage for elvisdragonmao's report on issue #716. Verified end-to-end against the local single-user docker stack: creating a contact with ``{organization, title, note, url, categories}`` and reading it back via ``nc_contacts_search_contacts`` returns every field populated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
7da72a3888 |
feat(api): log inbound User-Agent on management API and webhook receiver
Astrolabe (and any other PHP-side client) sends a stable User-Agent on
every outbound call to the MCP server. Capture it at the middleware
layer so backend access logs can attribute each request to a specific
client build — e.g. ``Nextcloud-Astrolabe/0.14.1``.
The middleware fires only for /api/v1/* and /webhooks/nextcloud,
which is the surface PHP-side clients hit; /mcp and /health stay
silent. The structured ``extra`` ({user_agent, http_method, http_path})
flows into OTel spans so the field is queryable in Grafana / Loki.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
fdcbd7bd3f |
feat(webhooks): add Deck card sync preset with vector indexing
Nextcloud Deck PR #7910 added IWebhookCompatibleEvent to CardCreated/ Updated/DeletedEvent and BoardUpdatedEvent, so Deck can finally emit real-time webhooks via core's webhook_listeners app. Wire this into the existing preset → parser → DocumentTask pipeline that already backs Notes / Calendar / Tables / Forms / Files sync. - Add deck_sync preset (app=deck, 4 events) and drop the stale "Deck does not support webhooks" comment. - Teach webhook_parser to convert Deck card events into DocumentTask(doc_type=deck_card, operation=index|delete) with stack_id metadata. BoardUpdatedEvent logs delivery at INFO and returns None — the polling scanner reconciles affected cards. - Cover three new unit tests for the deck create/delete/board-update paths plus symmetric fail-open tests for missing card.id / node.id in _parse_deck_event and _parse_file_event. The astrolabe admin UI auto-discovers the new preset via filter_presets_by_installed_apps(); no astrolabe-side wiring is required for it to appear in the Webhook Management card grid. Note: requires Deck ≥1.18.x (where PR #7910 lands); the preset is hidden when the Deck app isn't installed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandGitHub
|
1cf00d6be0 |
Merge pull request #719 from cbcoutinho/fix/contacts-create-dropped-fields-716
fix(contacts): persist all documented fields on create (fixes #716) |
||
|
Chris CoutinhoandClaude Opus 4.7
|
e0b7afb4b1 |
fix(embedding): instantiate BM25 singleton off the event loop
The ``BM25SparseEmbeddingProvider.__init__`` calls ``fastembed.SparseTextEmbedding(model_name="Qdrant/bm25")`` which downloads ~50 MB of model weights from HuggingFace and loads them into memory — observed >5 s wall-clock in production. The inference methods (``encode_async``, ``encode_batch_async``) already wrap work in ``anyio.to_thread.run_sync``, so the design intent is clearly to keep FastEmbed off the event loop. That protection just didn't cover the constructor. Symptom in the Astrolabe Cloud per-tenant deploy (deck #102 smoke): ~30–90 s after a user enables semantic search, the pod tips into a SIGKILL-restart cycle. Loki shows a single log line Initializing BM25 sparse embedding provider: Qdrant/bm25 followed by nothing else from the event loop until exitCode 137. Kubernetes ``/health/live`` httpGet probe timeout=5s fires 6 times in a row, kubelet kills the container, restart, repeat. Fix: switch ``get_bm25_service()`` to an async accessor that wraps the first-time construction in ``anyio.to_thread.run_sync``. Two existing call sites (``vector/processor.py:603``, ``search/bm25_hybrid.py:123``) update to ``await``. Both are already inside async functions so the await is free. New unit test pins the invariant by monkey-patching ``BM25SparseEmbeddingProvider.__init__`` with ``time.sleep(1)`` and asserting a concurrent ``anyio.sleep(0.05)`` finishes promptly — the test fails if the constructor ever runs back on the event loop. Same pattern exists in ``OllamaEmbeddingProvider.__init__`` (sync ``httpx.get`` health-check). Ollama isn't enabled in any current deploy; filed as a follow-up. Refs: - Astrolabe Cloud deck card #102 (smoke discovery) - Sibling fix #799 (NullPool for cross-loop-asyncpg, same class of "anyio bites you in production" bug) Verified: - ``uv run pytest tests/unit/`` — 1027 passed - ``uv run ruff check`` clean on touched files - ``uv run ty check`` clean on touched files - New tests pass Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
e98903c502 |
fix(storage): address review on PR #799 (stale comments, docs deprecation, unit test)
claude-review on #799 flagged: 1. Stale inline comment in ``initialize()`` (line 466) still said "Postgres uses a small bounded pool". Updated to reflect both backends now use NullPool. 2. Stale ``close()`` docstring referenced pool-size starving max_connections — irrelevant with NullPool. Replaced with the NullPool-aware rationale (dispose still tears down in-flight asyncpg connections cleanly). 3. ``docs/configuration.md`` actively directed operators to tune DATABASE_POOL_SIZE / DATABASE_MAX_OVERFLOW, with worked examples and pool math. Both are now deprecated no-ops; the table entries explain the deprecation and link to PR #799. Operators reading the docs will no longer be confused into tuning settings that don't do anything. 4. ``config.py`` comment for the deprecated fields updated to record the deprecation. Validators are intentionally kept (still reject < 1 / < 0) so misconfigured deploys fail loudly rather than silently — the reviewer flagged this as a minor UX wart but explicitly "not a blocker"; the docs change in (3) keeps operators away from the config altogether. 5. New ``tests/unit/test_storage_engine.py`` with three tests: - ``test_postgres_engine_uses_nullpool`` — pins ``isinstance( engine.pool, NullPool)`` so a refactor back to QueuePool / SingletonThreadPool can't silently re-introduce the cross- event-loop crashes. - ``test_postgres_engine_ignores_pool_sizing_settings`` — setting DATABASE_POOL_SIZE / DATABASE_MAX_OVERFLOW to huge values must not change pool type (proves the deprecated fields are wired-up no-ops). - ``test_postgres_engine_missing_asyncpg_driver_message`` — guards the existing actionable-error branch when the optional ``[postgres]`` extra isn't installed. Verified: - ``uv run pytest tests/unit/`` — 1028 passed - ``uv run ruff check`` clean on the touched python files Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
8cd3092e87 |
fix(storage): use NullPool for Postgres engine (cross-loop crashes under anyio TaskGroup)
The Postgres backend hit a hard crashloop in production deployments
where the MCP server runs under anyio TaskGroups with multiple
concurrent background tasks (`vector.oauth_sync.user_manager_task`,
processors, etc.) alongside the request-path code. Symptom in the
pod logs:
RuntimeError: Task <Task pending name='nextcloud_mcp_server.vector.oauth_sync.user_manager_task'>
got Future <Future pending cb=[BaseProtocol._on_waiter_completed()]>
attached to a different loop
followed seconds later by
RuntimeError: Event loop is closed
while SQLAlchemy's pool tries to clean up the failed connection.
The event loop becomes increasingly unresponsive as asyncpg
protocol Futures pile up holding references to closed loops; the
`/health/live` endpoint eventually misses its probe window and
the kubelet SIGKILLs the pod (exitCode 137), restart-looping the
backend.
Root cause: the engine was built with the default
`AsyncAdaptedQueuePool` (`pool_size=2, max_overflow=5`) and
`pool_pre_ping=True`. asyncpg connection objects are bound to the
event loop they were created on. When the process holds a
singleton engine and tasks running under different anyio
TaskGroups check out connections from that pool, the pre-ping
probe runs on a cached connection whose underlying transport
references a different loop's selector → cross-loop access →
crash.
Switch to `NullPool` — one fresh asyncpg connection per
`engine.connect()`, no caching, no cross-loop bookkeeping to get
wrong. asyncpg connection setup is ~5 ms over LAN and a single
round-trip in the local-Postgres case, so the throughput cost is
negligible for the MCP server's traffic shape (low concurrency,
bursty per-user requests). This matches what the SQLite branch
already does (see `initialize()`) and what Alembic's `env.py`
uses for migrations, so the codebase is now consistent across
all backends.
`DATABASE_POOL_SIZE` / `DATABASE_MAX_OVERFLOW` config knobs are
preserved for backward compatibility but no longer affect the
Postgres engine. The validators in `config.py` continue to
reject values < 1 / < 0, so misconfigured deploys still fail
loudly. A follow-up could mark them deprecated in
`docs/configuration.md`; out of scope here.
Discovered while smoke-testing the per-tenant Postgres flow in
Astrolabe Cloud (every-tenant pod fresh-provisions a database
via the ADR-026 backend → hits this crashloop within ~5 min of
the first MCP-routed request).
Refs:
- ADR-026 § "Concurrency model and pool sizing" (the original
QueuePool rationale, now superseded by this finding)
- nextcloud_mcp_server/alembic/env.py (NullPool for migrations)
- SQLAlchemy docs: NullPool is the documented choice when
connection objects don't survive across the lifecycle of the
pool's logical "owner" (here: the event loop)
Verified:
- `uv run ruff check nextcloud_mcp_server/auth/storage.py` clean
- `uv run pytest tests/unit/test_*storage*.py` → 29 passed
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
d717c64750 |
fix(storage): address PR #798 round-4 review (NOSONAR syntax + pg_advisory_lock + engine dispose + nits)
Addresses all 8 items in the round-4 bot review plus 4 remaining SonarQube OPEN issues that were silently broken by round 3's malformed NOSONAR markers. NOSONAR syntax fix (clears the remaining 4 OPEN SQ issues) ---------------------------------------------------------- Round 3 used ``# NOSONAR S<rule_key>`` form. SonarQube Python doesn't recognize the rule-key suffix — it treats the whole thing as a malformed suppression directive (S7632) AND lets the underlying rule keep firing (S7503 on ``_Cursor.__aenter__/__aexit__``). Switch every marker to bare ``# NOSONAR``, with the rationale moved into a preceding comment block. Affected sites: - storage.py: ``_Cursor.__aenter__``, ``_Cursor.__aexit__`` - config.py: ``get_database_ssl()`` ``return False`` + ``ssl.create_default_context()`` - test_storage_logging.py: ``SENTINEL_PASSWORD_FRAGMENT`` constant - test_storage_postgres.py: three ``bob_pw_v1`` / ``bob_pw_v2`` / ``carol_pw`` literals Bot 🔴#1 — defensive NOSONAR on get_database_ssl `return False` -------------------------------------------------------------- Bot predicted S4830 fires on the operator-opt-out path. SQ output shows it doesn't currently fire, but bare NOSONAR added defensively with rationale comment. Bot 🔴#2 — defensive NOSONAR on f-string SQL -------------------------------------------- ``update_oauth_session`` builds its SET clause via ``f"{', '.join(update_fields)}"``; ``get_audit_logs`` builds its WHERE clause via string concatenation. Both are safe (the fragments only come from this function's own branches, no user input), but the patterns trip taint analysers. Annotated both with bare NOSONAR + safety comment explaining the hardcoded-fragments invariant. Note: S2077 doesn't currently fire on these; defensive. Bot 🟡#3 — pg_advisory_lock for concurrent migrations ----------------------------------------------------- Without coordination, two pods rolling-updating simultaneously can both observe ``has_alembic=False`` and both try to apply migrations from scratch — the second crashes with "relation already exists". New ``_migration_lock()`` async context manager: - On Postgres: ``SELECT pg_advisory_lock(:lock_id)`` on a fresh connection (separate from the engine pool so it survives the ``to_thread.run_sync`` worker), held across BOTH the schema-inspect AND the migration call. Without that span, two pods could each observe "no alembic_version" before either started migrating, defeating the lock. - On SQLite: yields immediately (file-level locking serializes writes natively). Lock ID derived from ``sha256(b"nextcloud-mcp-server:migrations")[:8]`` as a stable signed int64 so we can't collide with other apps sharing the same Postgres. Bot 🟡#4 — RefreshTokenStorage.close() + lifespan wiring -------------------------------------------------------- New idempotent ``close()`` method calls ``await engine.dispose()``, nulls the engine, resets ``_initialized``. Wired into both ``app_lifespan_basic`` (BasicAuth) and the OAuth lifespan teardown, each wrapped in ``try/except Exception`` with ``logger.warning`` so a buggy dispose can't block SIGTERM. Without this, pooled asyncpg connections leak server-side slots until ``idle_in_transaction_session_timeout`` reaps them — with small pool defaults and frequent k8s rolling restarts this can starve ``max_connections``. Bot 🟢#5 — is_sqlite_url docstring on :memory: ---------------------------------------------- Updated docstring to note both file-backed and in-memory forms are recognized; caller is responsible for ``:memory:`` magic. Bot 🟢#6 — db_path via make_url(...).database --------------------------------------------- Replaced ``database_url.split("///", 1)[1]`` hack with SQLAlchemy's own URL parsing. Naturally handles in-memory (``.database is None`` → falls back to ``""``). Same lazy-import pattern as the existing ``mask_db_password`` to avoid module-import-time cost. Bot 🟢#7 — _to_sync_url unrecognized-driver guard ------------------------------------------------- Pulled ``_KNOWN_ASYNC_DRIVERS = ("aiosqlite", "asyncpg")`` into a module constant. When an unrecognized ``+<driver>`` token survives the strip, emits ``logger.warning`` with the known-supported list. Behavior unchanged for valid URLs. Bot 🟢#8 — get_audit_logs SELECT * → explicit columns ----------------------------------------------------- Replaced ``SELECT *`` with explicit column list. Future schema additions stay out of the dict return. New tests --------- - ``test_close_disposes_engine``: pins the public contract — engine nulled, state reset, second call is a no-op. - ``test_concurrent_initialize_serialized_by_advisory_lock``: spawns 3 concurrent inits against a fresh schema; asserts no "relation already exists" and exactly one ``alembic_version`` row at the end. Without the lock, this reliably fails on the second concurrent task. Docs ---- - ADR-026: new "Concurrent migrations across pods" subsection documents the advisory-lock approach + lock-ID derivation. Verification ------------ - ``uv run pytest tests/unit/`` — 1025 passed. - ``TEST_DATABASE_URL=… uv run pytest tests/integration/test_storage_postgres.py -m postgres`` — 9 passed (was 7). - ``ruff check && ruff format --check && ty check`` — clean. Expected post-push: SQ scan reports 0 OPEN issues (was 4). Tracked on Astrolabe Cloud POC board, card #99. --- _This PR was generated with the help of AI, and reviewed by a Human_ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
51419329b0 |
fix(storage): address PR #798 round-3 review (SonarQube + pool sizing + RETURNING test)
Round-3 fixes. Two threads: - 9 OPEN SonarQube issues caused the "E Security Rating on New Code" gate failure. The bot's diagnosis (sa.text(text_sql) → SQL injection) was a wrong guess; the actual SQ rules firing were different. - Bot's substantive concerns: pool defaults too aggressive, delete_browser_session RETURNING path untested on Postgres, schema_version legacy table created on Postgres, stale module docstring. - User's underlying question on the pool: "isn't 1 connection enough?" Right-sized to 2+5 and documented the concurrency model in ADR-026 so the rationale is durable. SonarQube quality-gate fixes (clears all 9 OPEN issues) ------------------------------------------------------- - BLOCKER S6418: rename `SECRET` constant in test_storage_logging.py to `SENTINEL_PASSWORD_FRAGMENT` + NOSONAR with rationale. - CRITICAL S3776: extract `_build_postgres_engine()` from `initialize()` (was complexity 26 > 15); incidentally creates a clean unit-test seam for engine args. - CRITICAL S4423: `ssl.create_default_context(cafile=...)` is flagged as "weak protocol" — Python 3.10+ already negotiates the strongest available protocol. Explicitly pass `purpose=ssl.Purpose.SERVER_AUTH` and NOSONAR with the Python-version rationale. - MAJOR S3358: split the TLS-mode nested ternary in the engine factory into a `_describe_ssl_arg()` helper. - MAJOR S2068 ×3: bind test app-password literals to local vars and put `# NOSONAR S2068` on the same line as the literal (anchoring requirement) instead of on the closing paren. - MINOR S7503 ×2: `# NOSONAR S7503` on `_Cursor.__aenter__/__aexit__` — they MUST be `async` per the context-manager protocol. Pool sizing right-sized (answers "why so many connections?") ------------------------------------------------------------ - `DATABASE_POOL_SIZE` default 10 → **2**. - `DATABASE_MAX_OVERFLOW` default 20 → **5**. - Per-pod max drops from 30 to 7. With 3 replicas, total = 21 connections (was 90) — well under managed-Postgres `max_connections=100`. - New INFO log at startup: `Postgres engine ready: pool_size=N max_overflow=M (per-pod max K connections)`. Surfaces the active sizing without grepping config. - New ADR-026 § "Concurrency model and pool sizing" explains asyncpg's single-flight connection semantics, the MCP workload shape (read-mostly point lookups), why-not-1 (multi-user serialization), and the tune-up/tune-down recipe. - `docs/configuration.md` table updated with new defaults + homelab-vs-prod tuning guidance, linking the ADR. RETURNING path covered on Postgres ---------------------------------- - New `test_browser_session_delete_returning` exercises the `DELETE … RETURNING user_id` path — the only RETURNING clause in the storage layer and the most dialect-sensitive SQL in this PR. Asserts both present-row (returns True, row gone) and absent-row (returns False) branches. Schema portability polish ------------------------- - `alembic 001`: gate `schema_version` table creation on `op.get_bind().dialect.name == "sqlite"`. The table exists purely to match the fingerprint of pre-Alembic SQLite databases; fresh Postgres installs no longer carry the dead legacy table. Misc polish ----------- - Module docstring: "SQLite-based" → "SQL-backed", with a sentence on the DATABASE_URL opt-in and an ADR-026 link. - Comment on `_wrap_row` noting `row._mapping` is the documented RowMapping accessor in SQLAlchemy 2.x despite the underscore. Skipped (rationale in PR reply) ------------------------------- - `_qmark_to_named` SQL-comment handling: docstring already notes the limitation; no `?` in storage SQL comments today. - Module-level `anyio.Lock()`: established precedent confirmed by the bot itself. - `get_audit_logs` `SELECT *`: pre-existing pattern, out of scope. Verification ------------ - `uv run pytest tests/unit/` — 1025 passed. - `TEST_DATABASE_URL=… uv run pytest tests/integration/test_storage_postgres.py -m postgres` — 7 passed. - `ruff check && ruff format --check && ty check` — clean. - Confirmed `schema_version` absent on fresh Postgres, still present on fresh SQLite. Tracked on Astrolabe Cloud POC board, card #99. --- _This PR was generated with the help of AI, and reviewed by a Human_ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
f2b7bf132f |
fix(storage): address PR #798 review feedback (credentials, asyncpg extra, TLS, pool)
Round-2 fixes after the bot review on PR #798 plus two user follow-ups (self-signed Postgres support; asyncpg should be a PyPI extra). Folded into the same PR rather than a follow-up since the work is still unmerged. Security -------- - Mask database credentials in all 5 log call sites (storage.py × 4, migrations.py × 1) via a new `mask_db_password()` helper in config.py. Uses SQLAlchemy's `make_url(...).render_as_string(hide_password=True)` with a regex fallback so the masking path never raises. - New `tests/unit/test_storage_logging.py` asserts a sentinel password never appears in `caplog` during `RefreshTokenStorage.initialize()`. Distribution ------------ - `asyncpg` moved to `[project.optional-dependencies] postgres` so a vanilla `pip install nextcloud-mcp-server` no longer pulls in the ~5 MB C extension. The Docker image runs `uv sync --extra postgres`, so containerized deployments are unchanged. - When `DATABASE_URL=postgresql+asyncpg://...` is set on a venv missing the extra, `RefreshTokenStorage.initialize()` raises a friendly RuntimeError pointing at `[postgres]` rather than the generic ModuleNotFoundError. TLS for the Postgres backend ---------------------------- - New `DATABASE_VERIFY_SSL` + `DATABASE_CA_BUNDLE` env vars mirror the existing `NEXTCLOUD_VERIFY_SSL` / `NEXTCLOUD_CA_BUNDLE` pattern (validators in Settings.__post_init__, `get_database_ssl()` helper alongside `get_nextcloud_ssl_verify()`). `DATABASE_VERIFY_SSL=false` wins over `DATABASE_CA_BUNDLE` for incident-response convenience. - Default is **None** rather than True — keeps PR #798's behavior intact for cluster-internal Postgres that runs without TLS. Operators opt into verify-full or supply a private CA. ADR-026 records the reasoning vs the Nextcloud HTTPS default. - Engine factory in `storage.py` passes `ssl` via `connect_args` only when `get_database_ssl()` returns non-None; otherwise asyncpg's default (`prefer`) applies. - Storage logs which TLS mode is active at INFO (no secret material). Configurable connection pool ---------------------------- - `DATABASE_POOL_SIZE` (default 10) and `DATABASE_MAX_OVERFLOW` (default 20) replace the hardcoded engine values. With many replicas this can blow past managed-Postgres `max_connections=100`; tune down for large fleets. - gte-1 / gte-0 validators in __post_init__ reject 0/negative pool sizes at startup with the offending value in the error. Consistency polish ------------------ - Migration 006: convert raw `op.execute("ALTER TABLE ... ADD COLUMN")` to `op.batch_alter_table(...).add_column(sa.Column("nonce", sa.Text))` for stylistic consistency with the rewritten 001-005. Downgrade now drops the column instead of being a no-op. - `registered_webhooks.created_at` standardized from `sa.Float` to `sa.BigInteger` (all other `*_at` columns); `store_webhook()` casts `time.time()` → `int`. - `is_sqlite_url()` made case-insensitive. Testing ------- - New `tests/integration/test_storage_postgres.py::test_cleanup_expired_roundtrip` exercises `cleanup_expired_tokens`, `cleanup_expired_sessions`, and `cleanup_expired_browser_sessions` — relies on DELETE rowcount, historically dialect-tricky. - `tests/unit/test_ssl_config.py` extended with `TestDatabaseSSLSettings` + `TestGetDatabaseSSL` classes (9 new tests) mirroring the existing Nextcloud SSL tests one-for-one. Docs ---- - `docs/configuration.md` Centralized-Storage section grew the four new env vars + a homelab example with a private CA. - `docs/ADR-026` grew Distribution, TLS, and `alembic/env.py` async-pattern subsections explaining the non-obvious design choices. Helm chart counterpart in cbcoutinho/helm-charts PR #34 (separate commit on `feat/nextcloud-mcp-server-database-url`). Verification ------------ - `uv run pytest tests/unit/` — 1025 passed. - `TEST_DATABASE_URL=... uv run pytest tests/integration/test_storage_postgres.py -m postgres` — 6 passed (including new cleanup test). - `uv run ruff check && uv run ruff format --check && uv run ty check -- nextcloud_mcp_server` — clean. Tracked on Astrolabe Cloud POC board, card #99. --- _This PR was generated with the help of AI, and reviewed by a Human_ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
292cbb3292 |
feat(storage): pluggable database backend via DATABASE_URL (ADR-026)
Adds a `DATABASE_URL` setting that lets `RefreshTokenStorage` run against
any SQLAlchemy async backend, primarily `postgresql+asyncpg://...` for
HA k8s deployments. Default behavior is unchanged: when `DATABASE_URL` is
unset the server falls back to the existing `TOKEN_STORAGE_DB` path /
ephemeral SQLite tempfile.
Why
---
Today every MCP pod needs its own PVC to hold the SQLite file, which
pins the Deployment to one replica and blocks horizontal scaling. With
this change, operators can point all replicas at a shared Postgres
(CNPG, RDS, etc.) and the pods become stateless. Encryption stays in
Python (Fernet); the database only sees ciphertext.
What changed
------------
- `config.get_database_url()` resolves DATABASE_URL → TOKEN_STORAGE_DB →
ephemeral tempfile in that priority order.
- `RefreshTokenStorage` builds a process-shared `AsyncEngine` in
`initialize()`. SQLite gets NullPool; Postgres gets pool_size=10,
max_overflow=20, pool_pre_ping=True. 30 aiosqlite call sites adapted
via a thin `_DBConn` / `_Cursor` / `_Row` / `_ExecuteCtx` shim so
existing method bodies need no churn beyond the connection
context-manager swap.
- 7 `INSERT OR REPLACE` statements rewritten as portable
`INSERT ... ON CONFLICT (...) DO UPDATE` (SQLite ≥ 3.24, Postgres ≥ 9.5).
- `sqlite_master` legacy-detection lookup replaced with SQLAlchemy
inspector so the path works against either backend.
- File-permission hardening + parent-dir creation gated on
`is_sqlite_url(...)` — centralized backends manage their own filesystem.
- Alembic migrations 001/002/003/005 converted from raw `op.execute(SQL)`
to portable `op.create_table()` / `op.create_index()` with SQLAlchemy
types. All timestamp columns are `sa.BigInteger` so Postgres allocates
BIGINT (unix epochs don't fit in INT4). SQLite treats BIGINT as
INTEGER, so existing deployments at revision 006 see no schema drift.
- `migrations.py` + CLI take URLs; `db {upgrade,downgrade,current,history}`
gain `--database-url / -u` alongside the legacy `--database-path / -d`.
`get_current_revision()` uses SQLAlchemy inspector instead of raw
sqlite3, so the CLI works against Postgres too.
- `docker-compose.yml` adds a `postgres-test` service under the
`postgres` profile (pinned `postgres:16-alpine` digest) for
integration testing.
- Unit storage tests parametrized over backends via shared
`tests/fixtures/storage_backend.py` — every test in
`test_app_password_storage.py` and `test_webhook_storage.py` runs
once per backend that is available. Postgres is opted in by
`TEST_DATABASE_URL`.
- New `tests/integration/test_storage_postgres.py` (5 tests, marked
`postgres` + `integration`) covers refresh-token, app-password,
OAuth-session, webhook, and audit-log paths end-to-end on Postgres.
- New `docs/ADR-026-pluggable-database-backend.md` records the decision;
`docs/configuration.md` documents `DATABASE_URL` with examples.
Out of scope
------------
- No SQLite → Postgres data migration tool (clean cutover; tokens reissue
on next login, webhooks re-register on next sync tick).
- This repo does not provision Postgres. The matching helm chart change
lives in cbcoutinho/helm-charts (database.url / existingSecret values).
Verification
------------
- `uv run pytest tests/unit/` — 1012 passed, SQLite path unchanged.
- `docker compose --profile postgres up -d postgres-test`
- `TEST_DATABASE_URL=... uv run pytest tests/integration/test_storage_postgres.py -m postgres -v`
— 5 passed.
- `TEST_DATABASE_URL=... uv run pytest tests/unit/test_app_password_storage.py
tests/unit/test_webhook_storage.py` — 50 passed (25 per backend).
- `uv run ruff check && uv run ruff format --check && uv run ty check -- nextcloud_mcp_server` — clean.
Tracked on Astrolabe Cloud POC board, card #99.
---
_This PR was generated with the help of AI, and reviewed by a Human_
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
79ea4e9e21 |
fix(config): emit background-ops advisory logs once per process
`_get_background_operations_enabled()` was emitting three advisory log lines (1 INFO + 2 deprecation WARNINGs) on every call. Because `get_settings()` is intentionally non-cached and runs on every MCP tool invocation via `get_client()`, the "Automatically enabled background operations for semantic search in multi-user mode" INFO line was firing per-request — 569 entries/hour in one production tenant. Gate the three log emissions behind a module-level `_bg_ops_advisories_logged` flag, mirroring the existing `_warn_missing_secret_once` precedent in `vector/webhook_receiver.py`. The boolean-derivation path stays unchanged, so the `Settings` value remains fresh per call. Extends the autouse `_reload_dynaconf_after_test` fixture to reset the new flag between tests, and adds two regression tests that call `get_settings()` five times and assert each advisory fires exactly once. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris Coutinho
|
55ca44c9ec |
Merge remote-tracking branch 'origin/master' into chore/lazy-logging-g004-sweep
# Conflicts: # nextcloud_mcp_server/vector/oauth_sync.py |
||
|
Chris CoutinhoandClaude Opus 4.7
|
65345fd6eb |
refactor: drop OAuth-refresh background-sync path from oauth_sync.py
Follow-up to #787/#789 (ADR-022 cleanup). After
`oauth_enabled ↔ enable_login_flow` became an invariant, the
`use_basic_auth=False` branch in `vector/oauth_sync.py` — and the
parameter wiring that fed it — was no longer reachable from any
supported deployment mode. This commit removes the dead code.
- nextcloud_mcp_server/vector/oauth_sync.py:
- Deleted `get_user_client_oauth` (the OAuth-token refresh helper) and
its `VECTOR_SYNC_SCOPES` constant.
- Deleted the `get_user_client` dispatcher. Internal callers now call
`get_user_client_basic_auth` directly.
- Dropped the `use_basic_auth: bool` parameter from `user_scanner_task`,
`multi_user_processor_task`, `_run_user_scanner_with_scope`, and
`user_manager_task`.
- Dropped the `token_broker` parameter from the same four functions —
they no longer need it now that the OAuth-refresh path is gone. The
`TokenBrokerService` constructed in `app.py` is still used by the
management API revoke endpoint, just not by background sync.
- Simplified the user-list query in `user_manager_task` to always read
from the `app_passwords` table.
- Replaced all `mode_label = "BasicAuth" if use_basic_auth else "OAuth"`
with a literal `[BasicAuth]` log prefix (keeps existing log filters
working).
- Updated the module docstring to describe the post-cleanup shape.
- Dropped the now-unused `TYPE_CHECKING` import of `TokenBrokerService`.
- nextcloud_mcp_server/app.py: dropped the `use_basic_auth = True` block
and the now-stale `token_broker if not use_basic_auth else None` /
`use_basic_auth` positional args from the two `tg.start(...)` calls in
the multi-user vector-sync lifespan. Token broker construction stays —
still consumed by the management API revoke endpoint via
`app.state.oauth_context["token_broker"]`.
- tests/integration/test_app_password_provisioning.py: deleted four tests
that exercised the now-removed OAuth-refresh path
(`test_oauth_mode_uses_refresh_token_only`,
`test_oauth_mode_raises_error_without_token`,
`test_get_user_client_oauth_function`,
`test_oauth_mode_requires_token_broker`) plus the
`test_get_user_client_dispatches_to_basic_auth` test for the deleted
dispatcher. Updated the module docstring + imports accordingly. The
BasicAuth-mode tests (`test_basic_auth_mode_uses_local_storage`,
`test_multiple_users_basic_auth_mode`, etc.) all remain.
No runtime-behaviour change in any supported deployment mode — the deleted
branches were already unreachable post-PR #787. 3 files changed,
+59 / -301; 1010 unit tests pass; integration jobs for
`mcp-login-flow` and `mcp-multi-user-basic` are the critical regression
gates before merge.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
665cb9b1eb |
refactor: convert f-string logging to lazy %-style format (G004)
Sweep all 1676 G004 violations across 112 files, converting
`logger.<level>(f"…{x}…")` to `logger.<level>("…%s…", x)`.
Why: ruff rule G004 was added to pyproject.toml to enforce lazy
%-style logging — defers formatting until the log level is enabled
and lets structured log tooling match the unformatted template.
Conversion preserves rendered output byte-for-byte:
- `{x}` → `%s` + `x`
- `{x!r}` / `{x!s}` / `{x!a}` → `%r` / `%s` / `%a`
- Format specs (`{x:.2f}`, `{x:>10}`) → `%s` + `format(x, 'spec')`
(printf-style specs aren't 1:1 with Python format specs, so we
delegate to `format()` to keep identical output)
- Literal `%` → `%%`
- Concatenated f-strings (`f"a {x} " "b"`) flattened
- Trailing kwargs (`exc_info=True`) preserved
Verified:
- `uv run ruff check --select G004` → 0 violations
- `uv run ty check -- nextcloud_mcp_server` → passes
- `uv run pytest tests/unit/` → 1010 passed
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
18db9d6cb3 |
refactor: prune dead pre-LOGIN_FLOW config/runtime branches
Two small post-merge cleanups deferred from PR #787 (ADR-022 follow-up). Both were explicitly noted in the reviewer's "acknowledged deferred items" list. 1. config.py: drop `enable_multi_user_basic_auth` and `enable_login_flow` from the dynaconf `_DEFAULTS` dict. They were removed from `_field_map` in PR #787, so `get_settings()` never read them anyway, but their presence in `_DEFAULTS` was visually misleading — readers might think they could be set via TOML when in fact `Settings.__post_init__` derives them from `MCP_DEPLOYMENT_MODE`. Replaced with a NOTE comment pointing at the canonical derivation site. 2. app.py: the lifespan code had `use_basic_auth = not oauth_enabled or settings.enable_login_flow`, which became always-True once PR #787 enforced `oauth_enabled ↔ enable_login_flow` via __post_init__. Hard-coded to `True` with a comment explaining the invariant and pointing at the separate follow-up that will prune the now-unreachable `use_basic_auth=False` code paths in `vector/oauth_sync.py` (which includes deleting the `use_basic_auth` parameter from `user_manager_task` / `oauth_processor_task` and the OAuth-token-refresh branch in `get_user_client`). Kept the variable name and the call-site conditionals as-is for now so that follow-up is a clean mechanical diff. No runtime behaviour change: `use_basic_auth` already evaluated to True in every supported mode after PR #787, and the `_DEFAULTS` entries were already shadowed by `__post_init__`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
0fb21b5c6d |
chore: address review-round-5 — stale _sync_derived_flags reference + missing migration-hint test
Two findings from the reviewer's latest pass: - config_validators.py:329: comment in the LOGIN_FLOW validation block still referenced `_sync_derived_flags` (removed in commit 5; derivation now lives in `Settings.__post_init__`). Updated the comment to point at the correct location so a future reader grepping for the function name doesn't come up empty. - tests/unit/test_config_validators.py: added `test_oauth_single_audience_migration_hint` next to the existing `test_invalid_deployment_mode_raises_error`. The new test pins the ADR-022 rename-hint branch in `detect_auth_mode` by setting `MCP_DEPLOYMENT_MODE=oauth_single_audience` and asserting the ValueError mentions both the old and new mode names plus "ADR-022". Without this, a future refactor could drop the hint without any test catching it (the prior `invalid_mode` test only asserts the generic "Valid values:" prefix). No functional changes; 1010 unit tests now pass (+1 from the new hint test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
1fa4c82fd2 |
chore: address review-round-4 nits — stale delenv, upgrade hint, in-sync notes
Four small follow-ups from the reviewer's latest pass:
- tests/unit/test_stdio.py:18: the single_user_env fixture used
monkeypatch.delenv("ENABLE_MULTI_USER_BASIC_AUTH", ...). That env var
is no longer read after the ADR-022 follow-up; switched to delenv of
MCP_DEPLOYMENT_MODE which is the canonical mode-selection input today.
Comment updated to match.
- config_validators.py: when detect_auth_mode rejects an invalid
MCP_DEPLOYMENT_MODE, surface a one-line ADR-022 migration hint if the
rejected value is exactly "oauth_single_audience" (the most common
upgrade pain — users carrying that value over from ADR-021 .env files).
Other invalid values get the regular "Valid values: …" message
unchanged.
- config.py + config_validators.py: added cross-reference comments on
both mode-resolution sites (Settings.__post_init__ and
detect_auth_mode) noting that they each compute the canonical mode
independently and must be kept in sync when a new mode is added.
Surfaces the parallel-duplication intentionally so the next maintainer
doesn't have to discover it.
- docs/ADR-021-configuration-consolidation.md:92: appended a trailing
comment to the historical "valid values" example, marking
oauth_single_audience and oauth_token_exchange as removed in ADR-022.
ADR-021 stays as the historical record; the trailer points future
readers at the current state.
No functional changes; 1009 unit tests still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
ade42b55dc |
docs: clear review-round-3 nits — stale login_flow_v2, duplicates, field comments
Five small findings from the reviewer's third round, plus a SonarCloud
quality-gate failure on a test fixture.
- docs/troubleshooting.md, docs/configuration.md: six pre-PR references
to a non-existent `login_flow_v2` mode value (the actual enum value is
`login_flow`). They predated this PR but became actively misleading
once `detect_auth_mode` started raising ValueError for anything not in
the mode_map. Replaced with `login_flow` via sed.
- docs/configuration-migration-v2.md: removed a duplicate
`MCP_DEPLOYMENT_MODE=multi_user_basic` line in the troubleshooting
section (around line 447) — same shape as the round-2 duplicate
caught earlier in the migration-steps section. Also dropped the
`oauth_token_exchange` row from the mode-value table around line 364
(that enum value was removed in
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
6e7c821761 |
fix(config): derive mode flags in Settings.__post_init__; address review round 2
The integration jobs for `mcp-multi-user-basic` and `mcp-login-flow`
were failing with HTTP 500s. Root cause: `get_settings()` builds a
fresh Settings on every call (not cached). Commits 3 and 4 set the
derived `enable_login_flow` / `enable_multi_user_basic_auth` flags as
a side effect of `detect_auth_mode`. detect_auth_mode runs once at
startup, against the Settings instance owned by `validate_configuration`.
Every per-request call site that does `settings = get_settings()` got
a fresh Settings with both flags at their default `False` (since the
env-var aliases were dropped), causing the multi-user dispatcher in
`context.py` to take the wrong branch and crash.
Fix: move the derivation into `Settings.__post_init__`. Every Settings
instance now carries correct flags from the moment it's constructed —
no caching needed, no mutation-after-construction race. detect_auth_mode
becomes a pure reader of the already-derived state.
The legacy env-var deprecation check moves with it. It also picks up
the reviewer's truthy-string fix: previously `os.getenv(legacy)` fired
for the literal string "false" (a non-empty Python string is truthy),
which would have errored on any user with a leftover
`ENABLE_LOGIN_FLOW=false` in their `.env`. The check now only fires
when the value lowercases to one of {"1", "true", "yes", "on"}.
- nextcloud_mcp_server/config.py: extend Settings.__post_init__ with
the legacy-deprecation block and the derived-flag derivation
(resolve mode from deployment_mode + username/password, set flags).
- nextcloud_mcp_server/config_validators.py: drop the
`_sync_derived_flags` helper (superseded by __post_init__). Drop the
legacy-env-var deprecation block (moved). `detect_auth_mode` is now
pure — no mutation. Drop the now-unused `import os`.
- tests/unit/test_config_validators.py: legacy-env-var tests now
expect `ValueError` at `Settings(...)` construction (via `get_settings()`),
not at `detect_auth_mode` call. Added two new tests:
* `test_legacy_env_var_check_ignores_falsy_strings` — pins the
truthy-string fix (reviewer round 2 finding).
* `test_derived_flags_stable_across_get_settings_calls` — regression
test pinning the integration-test fix (two consecutive
`get_settings()` calls return Settings instances with the same
derived flags).
Also reworked `test_login_flow_mode_auto_derives_enable_login_flow_flag`
to assert at-construction derivation (not the old mutation pattern).
- docs/configuration-migration-v2.md: dropped the duplicate
`MCP_DEPLOYMENT_MODE=multi_user_basic` line (review round 2 nit — a
sed artifact from commit 4).
- docs/ADR-021-configuration-consolidation.md: sed-replaced the in-body
`MCP_DEPLOYMENT_MODE=oauth_single_audience` examples with `login_flow`
(review round 2 nit — only the status header was updated in commit 4).
- tests/conftest.py: docstring comment for the multi-user-basic fixture
switched from `ENABLE_MULTI_USER_BASIC_AUTH=true` to
`MCP_DEPLOYMENT_MODE=multi_user_basic` (review round 2 nit).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
282c245da1 |
refactor(config)!: drop ENABLE_MULTI_USER_BASIC_AUTH env var, fail loud on legacy aliases
Same pattern as the ENABLE_LOGIN_FLOW removal in the previous commit:
the deployment mode (MCP_DEPLOYMENT_MODE) is the single source of truth
for selecting an auth flow. The ENABLE_MULTI_USER_BASIC_AUTH env-var
alias is redundant with `MCP_DEPLOYMENT_MODE=multi_user_basic`.
Unlike the ENABLE_LOGIN_FLOW removal — where silent removal was safe
because Login Flow v2 is the auto-detection default — silent removal
here would be a surprise: a user with only ENABLE_MULTI_USER_BASIC_AUTH=true
in their .env would auto-detect into LOGIN_FLOW after upgrade (wrong
runtime mode). Mitigation: detect_auth_mode now reads os.environ
directly for both legacy aliases and raises ValueError with a one-line
migration message if either is set. Applied retroactively to
ENABLE_LOGIN_FLOW as well — loud is better than silent.
- nextcloud_mcp_server/config.py:
- Drop the dynaconf env-var alias entry for ENABLE_MULTI_USER_BASIC_AUTH.
- Update the `enable_multi_user_basic_auth` field docstring to mark it
as derived / not user-settable.
- `_is_multi_user_mode()` (early-config helper, runs before Settings
is built) switched to checking MCP_DEPLOYMENT_MODE directly. Now
consistent with the canonical detection in detect_auth_mode.
- nextcloud_mcp_server/config_validators.py:
- Drop the auto-detection branch (`if settings.enable_multi_user_basic_auth`).
Selection of MULTI_USER_BASIC is now exclusively via the explicit
MCP_DEPLOYMENT_MODE branch.
- Add `enable_multi_user_basic_auth` to `_sync_derived_flags` alongside
`enable_login_flow` — both flags are now derived from the resolved mode.
- Drop `enable_multi_user_basic_auth` from
`MODE_REQUIREMENTS[MULTI_USER_BASIC].required` and from the
`forbidden` lists of SINGLE_USER_BASIC and LOGIN_FLOW (no longer
user input → no meaningful forbidden check).
- Add loud-deprecation `ValueError` block at the top of detect_auth_mode
that errors with a clear migration message when ENABLE_MULTI_USER_BASIC_AUTH
or ENABLE_LOGIN_FLOW is found in os.environ.
- tests/unit/test_config_validators.py:
- Switch ~10 fixtures from `enable_multi_user_basic_auth=True` to
`deployment_mode="multi_user_basic"` (mirrors `enable_login_flow`
treatment from the previous commit).
- Switch two `patch.dict(os.environ, {"ENABLE_MULTI_USER_BASIC_AUTH": "true"})`
blocks to use MCP_DEPLOYMENT_MODE.
- Rename `test_forbidden_multi_user_basic_auth` to
`test_forbidden_multi_user_basic_when_credentials_present` — the
scenario is now an explicit-mode + credentials conflict, not an
env-var-flag conflict.
- Add `test_legacy_enable_multi_user_basic_auth_env_var_errors` and
`test_legacy_enable_login_flow_env_var_errors` to exercise the new
loud-deprecation ValueError path.
- docker-compose.yml: mcp-multi-user-basic profile switched to
`MCP_DEPLOYMENT_MODE=multi_user_basic`.
- env.sample: replaced `#ENABLE_MULTI_USER_BASIC_AUTH=true` example with
`#MCP_DEPLOYMENT_MODE=multi_user_basic`.
- docs/authentication.md, configuration.md, troubleshooting.md,
auth-flows.md, webhook-management-guide.md,
configuration-migration-v2.md, ADR-025: replaced env-var examples
with the canonical MCP_DEPLOYMENT_MODE form.
- docs/ADR-020: marked partly superseded by ADR-022.
- CLAUDE.md: Multi-User BasicAuth section updated to set
MCP_DEPLOYMENT_MODE.
- nextcloud_mcp_server/vector/oauth_sync.py: module docstring updated.
BREAKING CHANGE: ENABLE_MULTI_USER_BASIC_AUTH is no longer read from
the environment, and setting it now raises a startup ValueError with
a migration message. Replace `ENABLE_MULTI_USER_BASIC_AUTH=true` with
`MCP_DEPLOYMENT_MODE=multi_user_basic`. The same loud-deprecation
check is also applied to the recently-removed ENABLE_LOGIN_FLOW —
replace with `MCP_DEPLOYMENT_MODE=login_flow` (or drop both;
`login_flow` is the auto-detect default when no other auth env vars
are set).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
df4994e860 |
refactor(config)!: derive enable_login_flow from mode, remove ENABLE_LOGIN_FLOW env var
Once OAUTH_SINGLE_AUDIENCE was renamed to LOGIN_FLOW and the validation
gate ensured the only meaningful configuration was
`MCP_DEPLOYMENT_MODE=login_flow + ENABLE_LOGIN_FLOW=true`, the two
controls became redundant. Setting the mode is sufficient; the
ENABLE_LOGIN_FLOW env var doesn't add information.
This commit makes the deployment mode the single source of truth for
the Login Flow v2 toggle:
- `nextcloud_mcp_server/config.py`: drop the `ENABLE_LOGIN_FLOW`
dynaconf env-var alias. The `enable_login_flow` field stays as an
internal attribute so the 6 runtime call sites (app.py x4,
context.py, auth/scope_authorization.py) keep working unchanged.
Updated field docstring to flag it as derived.
- `nextcloud_mcp_server/config_validators.py`:
- Drop `enable_login_flow` from `MODE_REQUIREMENTS[LOGIN_FLOW].required`.
- Drop the validation gate that required ENABLE_LOGIN_FLOW=true for
LOGIN_FLOW mode (no longer possible to misconfigure — the flag is
derived, not user input).
- Add `_sync_derived_flags()` helper called at every return path of
`detect_auth_mode` to set `settings.enable_login_flow` from the
resolved mode.
- `tests/unit/test_config_validators.py`: drop `enable_login_flow=True`
from happy-path fixtures (no longer needed — detection sets it).
Repurpose `test_login_flow_requires_enable_login_flow_flag` into
`test_login_flow_mode_auto_derives_enable_login_flow_flag` which
asserts the new auto-derivation behaviour for both LOGIN_FLOW and a
non-LOGIN_FLOW mode.
- `docker-compose.yml`: remove `ENABLE_LOGIN_FLOW=true` from the
`mcp-login-flow` and `mcp-keycloak` profiles.
- `env.sample`: remove the ENABLE_LOGIN_FLOW reference; the comment
on `MCP_DEPLOYMENT_MODE` now notes the derived flag.
- `docs/configuration.md`, `docs/authentication.md`,
`docs/login-flow-v2.md`, `docs/auth-flows.md`,
`docs/troubleshooting.md`, `docs/ADR-025-*.md`: replace
ENABLE_LOGIN_FLOW=true examples and references with
MCP_DEPLOYMENT_MODE=login_flow.
BREAKING CHANGE: `ENABLE_LOGIN_FLOW` is no longer read from the
environment. Anyone who relied on `ENABLE_LOGIN_FLOW=true` to activate
Login Flow v2 should set `MCP_DEPLOYMENT_MODE=login_flow` instead (or
rely on it being the default when no other auth env vars are set).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
cafd318f36 |
refactor(config)!: rename OAUTH_SINGLE_AUDIENCE to LOGIN_FLOW, gate on ENABLE_LOGIN_FLOW
The AuthMode.OAUTH_SINGLE_AUDIENCE enum was a vestige of ADR-021's
original design where it co-existed with OAUTH_TOKEN_EXCHANGE. The
un-augmented OAuth bearer pass-through it represented relied on
Nextcloud-side patches to user_oidc (Bearer token validation on
non-OCS endpoints) that were never merged upstream (see
docs/authentication.md, docs/login-flow-v2.md). The working path —
mcp-login-flow profile — sets ENABLE_LOGIN_FLOW=true on top of this
mode so Login Flow v2 acquires per-user Nextcloud app passwords via
a browser flow. With OAUTH_TOKEN_EXCHANGE removed in
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
9d5ac01f24 |
fix(calendar): preserve floating/TZID semantics across CalDAV roundtrip (#782)
The CalDAV REPORT in `_search_events_by_date` unconditionally requested server-side `<C:expand>`. Per RFC 4791 §9.6.5 the server then normalizes every expanded DTSTART/DTEND to UTC `Z`, which destroyed two pieces of information on the read path: - RFC 5545 floating local times came back as fake-UTC (a `+00:00` suffix that did not match the stored value), so a 2:30 PM floating event was indistinguishable from a 14:30 UTC event in the MCP response. - TZID-bound events lost their IANA TZID context — a "10am America/New_York" event came back as `14:00:00+00:00`, making it impossible for callers to reconstruct DST-aware recurrence semantics. Replace `<C:expand>` with client-side recurrence expansion via the `recurring-ical-events` library (promoted from transitive to direct dep), so the wire response retains its original DTSTART format. Surface the TZID parameter as new `start_tz`/`end_tz` fields on `CalendarEventSummary`. Add an optional `timezone` (IANA name) parameter to `nc_calendar_create_event` and `nc_calendar_update_event` so callers can pin a TZID for naive input; the helper attaches `ZoneInfo(...)` and emits a paired `VTIMEZONE` component. Naive input without `timezone` continues to store as RFC 5545 floating local time (with a warning logged). Offset-aware input continues to store as UTC `Z`. Drive-by: switch the update path's DTSTART/DTEND assignment from raw `datetime` to `vDDDTypes(dt)` wrappers — the previous code produced invalid iCal like `DTSTART:2026-05-14 10:00:00+00:00` for any TZ-aware update. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris Coutinho
|
9072559d26 | chore: Address reviewers feedback | ||
|
Chris CoutinhoandClaude Opus 4.7
|
271904c227 |
fix(deck): address review — notesPath key, scopes, modernize types
PR #781 review round 1: - 🔴 Fix notesPath key: the Notes API returns the folder under camelCase ``notesPath`` (see models/notes.py:43), but `deck_attach_note` was looking up snake_case ``notes_path`` and silently falling back to ``"Notes"``. Users with a non-default notes folder would have produced shares pointing at non-existent files (404 on click in Deck UI). - 🔴 Add wire-through unit test that would have caught the above: extract `_resolve_note_attach_path(client, note_id)` as a testable helper that encapsulates the camelCase-key lookup. Three new tests: custom notesPath honored, missing key falls back to default, null category handled. - 🟡 Modernize new fields on `DeckAttachmentExtendedData` to PEP 604 (`X | None`) per CLAUDE.md. - 🟡 Drop unnecessary string forward reference on `ListAttachmentsResponse.results` — DeckAttachment is defined earlier in the same module. - 🟢 Move `pytestmark = pytest.mark.unit` to module level in test_sharing_client.py to match the convention in test_deck_server.py. Per user request: `deck_attach_file` is now scoped `deck.write` + ``files.read`` (was just `deck.write`) so the generic file-share permission story is consistent — only `deck_attach_note` keeps `notes.read` since it specifically reads from the Notes app. Docstring updated to emphasise the tool is generic over the user's Files (PDFs/images/etc., not just markdown). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
c0a974c498 |
feat(deck): add file/note attachment MCP tools
Adds four tools that expose Deck card attachments via the MCP surface: deck_attach_file, deck_attach_note, deck_list_attachments, and deck_delete_attachment. The attach* variants share an existing Files entry (or Notes-app note) with the card via OCS shareType=12 — same mechanism the Deck UI's "Share from Files" picker uses, no file copy. This replaces the prior workaround of appending bulky activity content as Deck card comments: per-PR/per-event narrative now lives in NC Notes and surfaces on the tracking card as a clickable attachment that opens the original note in place. Implementation reuses existing client methods (SharingClient.create_share, DeckClient.get/delete_attachment, NotesClient.get_settings/get_note); no new client code. _SHARE_TYPE_DECK is centralised with a CI-guard test to prevent silent drift, and SharingClient.create_share's wire format is pinned to what the Deck Vue source sends. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
9cb0b33ec1 |
fix(health): forward api-key to Qdrant /readyz so Cloud probes work
The readiness handler called the configured `qdrant_url/readyz` with a
bare httpx.AsyncClient — no headers. That works against a self-hosted
Qdrant (where /readyz is anonymous), but Qdrant Cloud's auth gateway
returns 403 for any unauthenticated request, including /readyz, /livez
and /healthz. Result: every probe against a Cloud cluster fell into
the "status 403" branch, the handler returned 503, and the Pod never
went Ready — even when the configured `AsyncQdrantClient` itself was
authenticating fine for actual collection traffic.
Forward `settings.qdrant_api_key` as the `api-key` header (mirroring
what `vector/qdrant_client.py:540` already does for the real client).
When the key is unset (self-hosted, anonymous case) we send no header,
so existing self-hosted deployments are unchanged.
Verified end-to-end against Qdrant Cloud:
- Without header: GET /readyz -> 403 {"error":"forbidden"}
- With api-key: same request shape returns 200 (matches what
AsyncQdrantClient.wait() relies on internally).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Chris Coutinho
|
f3a66cf6bb | chore: ruff format | ||
|
Chris Coutinho
|
189024f1ca | Merge remote-tracking branch 'origin/master' into fix/qdrant-doc-id-keyword-index | ||
|
Chris CoutinhoandClaude Opus 4.7
|
8246d9a088 |
fix(vector): address PR review round 17 + local-mode collection-creation regression
Round 17 reviewer (🟡 Important): 1. docs/configuration.md degraded-migration runbook said `doc_id backfill failed on …` but the actual log line in qdrant_client.py:415 is `doc_id backfill scroll failed on …`. Operators grepping the runbook string would have missed it. Insert the `scroll` qualifier. 2. _create_one_payload_index returned True on the 400 schema-conflict path, so a wrong-type index discovered at create time skipped the consolidated `Payload index creation incomplete` summary — but a wrong-type index discovered via the existing-schema check at line 195-206 did fire it. Tenants whose payload_schema is hidden from their JWT (Qdrant Cloud collection-scoped tokens) only ever observe the create-time path, so they never saw the operator-level summary. Return False so the summary fires in both cases. 3. docs/configuration.md said the upgrade-time delay was `proportional to point count while writes are issued` — overstating the cost. Writes are proportional to int-typed points only; the scroll itself is proportional to total point count. Reword. Local-mode collection-creation regression (root-cause of failing single-user / login-flow / multi-user-basic CI jobs): PR #779 changed the existence probe in get_qdrant_client from collection_exists() (returned bool in both modes) to get_collection() + except UnexpectedResponse(status_code=404). The HTTP-mode client raises UnexpectedResponse with a 404 body, but the local/in-memory client raises ValueError(f"Collection {name} not found") — see qdrant_client/local/async_qdrant_local.py. The narrow except clause let the ValueError propagate, app.py's lifespan re-raised as RuntimeError, and the mcp container crashed on first start. Catch ValueError too, with a `not found` substring guard so genuine programming bugs (bad collection_name, etc.) still surface. Tests: extend the existing 400-path test to assert the new failed_fields contract; add two get_qdrant_client unit tests pinning the local-mode VE catch (positive case + propagation case). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandGitHub
|
bf19c948e2 |
Merge pull request #769 from KuriGohan-Kamehameha/feat/contacts-search-contacts
feat(contacts): add nc_contacts_search_contacts free-text search tool |
||
|
Chris Coutinho
|
363c2a2624 |
Merge remote-tracking branch 'origin/master' into fix/qdrant-doc-id-keyword-index
# Conflicts: # nextcloud_mcp_server/vector/qdrant_client.py |
||
|
Chris CoutinhoandClaude Opus 4.7
|
8f4f5c0079 |
fix(vector): address PR review round 16 — type-aware index check, comments
Detect pre-existing payload indexes with the wrong schema type in `_ensure_payload_indexes`. The previous "field already in existing_schema → skip" branch silently survived a collection migrated from the int-doc_id era where `doc_id` is indexed as INTEGER, letting `MatchValue(value="123")` searches keep failing with HTTP 400 on Qdrant Cloud strict mode — exactly the production failure this PR was meant to fix. New behaviour: compare `existing_schema[field].data_type` against the declared type; on mismatch log a WARNING and append to `failed_fields` so the consolidated end-of-function summary picks it up. No auto-repair (operator intervention only — see docs/configuration.md recovery procedure). New test exercises the doc_id-INTEGER scenario end-to-end and asserts both the per-field WARNING and the summary line. Clarify the `_verify_news_items` malformed-doc_id rationale: the news API has no per-item endpoint, so a malformed doc_id genuinely cannot be verified against the source of truth. We err toward false-positive (keep) over false-negative (drop) — same conservative posture as `_verify_notes` and `_verify_deck_cards`. The producer-side validation is the real security boundary; the verifier is defence-in-depth. Both the inline comment and the WARNING message now spell this out. Add a TODO in `get_last_indexed_timestamp` flagging the O(N) cost on every incremental sync tick. The previous single-page `limit=10_000` silently bounded the scroll; paginating fixed correctness but made the unbounded cost visible. The follow-up tracker (canonical TODO at `api/visualization.py`) covers migrating the max-`indexed_at` to a sentinel point or collection metadata for O(1) lookup. Consolidate the duplicate non-numeric-doc_type TODOs at `api/visualization.py:508` and `auth/viz_routes.py:570` into a single canonical comment in `visualization.py`; `viz_routes.py` is reduced to a back-reference. Removes the rot risk of "fixed in one place, forgotten in the other." The canonical comment also references the O(1) timestamp follow-up in `scanner.py`. Document the `batch_size = 256` (qdrant_client.py) vs `_DELETION_TRACKING_PAGE_SIZE = 1024` (scanner.py) split with cross-referencing comments at each site: the smaller batch is for the read-write backfill upsert path (Qdrant accepts ~256-point chunks comfortably); the larger page is for read-only deletion-tracking scrolls where no per-page write round-trip applies. Replace `assert qdrant_client is not None` in `scan_user_documents` with `cast(AsyncQdrantClient, qdrant_client)` plus an explanatory comment. `assert` is silently elided under `-O`; `cast` is the conventional zero-cost narrower for branches the type checker can't infer from the surrounding `if not initial_sync` ternary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
04bc2325f2 |
fix(qdrant): use get_collection for startup probe (multi-tenant safe, take 2)
Follow-up to PR #778 — `collection_exists()` is also denied by Qdrant Cloud on a collection-scoped JWT, so the multi-tenant fix needs to go one step further: use `get_collection(name)` (the underlying GET `/collections/{name}` call) and treat a 404 `UnexpectedResponse` as the "doesn't exist" signal. That endpoint is the only existence-probe Qdrant permits on a collection-scoped JWT — listing or probing collection metadata cluster-wide is a tenant-isolation boundary by design. Hit during Astrolabe Cloud smoke17 with the post-#778 image: qdrant_client.http.exceptions.UnexpectedResponse: 403 (Forbidden) raw response: {"error":"forbidden"} File "qdrant_client.py", line 84, in get_qdrant_client collection_present = await _qdrant_client.collection_exists(...) Folds the existence check into the same `get_collection()` call that already runs immediately afterward for dimension validation, so the new path is also one fewer round-trip on the happy path. Cold-start (collection genuinely missing) behavior is unchanged: 404 → `collection_info` is None → fall through to `create_collection()`. Whether `create_collection` succeeds is an orthogonal concern (managed multi-tenant setups pre-provision collections externally; admin-key single-tenant setups can create on the fly). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
f2d4982b2f |
fix(qdrant): use collection_exists for startup probe (multi-tenant safe)
The startup path in `get_qdrant_client()` calls `get_collections()` to
check whether the configured collection already exists. That's a
cluster-wide list operation; in managed multi-tenant Qdrant Cloud
deployments where each tenant's JWT is scoped to a single collection
(by design — `access: [{"collection": "tenant_<id>", "access": "rw"}]`),
the call returns `403 Forbidden` and the FastAPI lifespan crashes:
qdrant_client.http.exceptions.UnexpectedResponse: 403 (Forbidden)
raw response: {"error":"forbidden"}
RuntimeError: Cannot start vector sync - Qdrant initialization failed
Switching to `collection_exists(collection_name)` (per-collection
HEAD-style probe) only requires access to the named collection, which
the tenant JWT has. Single-tenant deployments using an admin/master
key are unaffected — they had access to both forms; this picks the
narrower one.
Doesn't change creation semantics: when the collection isn't present
the code path still calls `create_collection`. In a managed setup
where the collection is pre-provisioned by an external admin (e.g.,
the Astrolabe Cloud control plane's create-tenant workflow), that
branch never fires for an existing tenant; cold-start tenants get
their collection created by the workflow before the Pod boots.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|