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>
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>
Resolves both 🟡 important issues from the latest review:
1. `page_number` was unconditionally overwritten in `viz_routes.py:696` even
when Qdrant's payload lacked the field, clobbering the value resolved
from `chunk_context.page_number`. The new helper returns each field
independently and both call sites only overwrite via `is not None`
guards, matching the existing logic in `visualization.py`.
2. The ~60-line `if chunk_index is not None: ... else: ...` Qdrant scroll
block was duplicated between `api/visualization.py` and
`auth/viz_routes.py`. Extracted into `get_chunk_bbox_and_page_from_qdrant`
in `search/context.py` alongside the existing private `_get_chunk_*_from_qdrant`
helpers; both routes now share ~12 lines of caller code.
New unit tests at `tests/unit/test_chunk_bbox_helper.py` cover the indexed
and offset paths, the `(bbox, None)` regression case, and graceful
degradation on Qdrant strict-mode 400 (which also closes nit #4).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`<d:href>` is required by RFC 3986 to be percent-encoded, so non-ASCII
filenames (e.g. Chinese, Cyrillic) were leaking through `list_directory`
and the SEARCH-based tools (`find_by_name`, `find_by_type`,
`list_favorites`, `search_files`) as their URL-encoded form. Decode with
the already-imported `urllib.parse.unquote` before exposing to callers.
Fixes#776
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Remove unreachable doc_type=="file" branch (and pymupdf/pymupdf4llm
imports) from _fetch_document_text in search/context.py — the file
path is short-circuited in get_chunk_with_context before reaching it.
- Drop the redundant `username = request.user.display_name` alias in
auth/viz_routes.py; both Qdrant scroll filters now reference user_id
consistently with the rest of the handler.
- Add TestAdjacentChunkBoundary in tests/unit/test_chunk_context_offset_gate.py
covering chunk_index=0 (before-fetch gate closed) and
chunk_index=total_chunks-1 (after-fetch gate closed) — the two
off-by-one boundaries previously untested.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- search/context.py: rename triple-negation gate condition to
`skip_offset_lookup` named boolean for readability; convert new
logger.warning to lazy %-style per repo convention.
- api/visualization.py, auth/viz_routes.py: add comment on the offset-only
Qdrant scroll branch noting it is a legacy path for pre-astrolabe#75
clients and degrades gracefully on Qdrant Cloud strict mode.
Reviewer item #2 (extracting the duplicated scroll block into a shared
helper) deferred to a follow-up issue per the reviewer's "not blocking"
framing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- pdf_highlighter.compute_chunk_bboxes_batch: drop unused chunk_text
destructure (SonarQube finding), and replace positional
page_boundaries[page_num - 1] with a key-based next() match so
reordered or non-1-indexed boundaries can't silently shift the bbox.
Convert touched f-string log to lazy %s formatting.
- vector/processor: rename the trace_operation span from
"vector_sync.generate_highlights" to "vector_sync.compute_chunk_bboxes"
to match what the function actually does.
- Add test_compute_chunk_bboxes_handles_unordered_page_boundaries —
reverses the boundaries list and asserts identical results to the
in-order case, guarding the boundary-lookup regression class.
- Pin pre-push-review skill to sonnet model.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- chunk_bboxes is now dict[int, list[tuple[...]]] holding the bbox list
directly, not {"bbox": ..., "page": ...}. The page from text-search was
stored but never read; page_number from offset-based assignment is
authoritative for the Qdrant payload.
- Add two unit tests for the documented omission contract: chunks whose
offsets fall outside every page boundary, and chunks whose text cannot
be located on the rendered page, are silently dropped from the result.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the deferred half of PR #767 review issue 2: instead of just
documenting the "0/N misreport" with a logger.warning, propagate the
caller's None for chunk_index through the dataclass, position markers,
and response builders so callers can distinguish "unknown position"
from "actually chunk 0".
Changes:
- ChunkContext.chunk_index: int → int | None
- _insert_position_markers: chunk_index parameter is int | None; when
None, renders "Chunk ?/N" instead of "Chunk 1 of N"
- get_chunk_with_context: drops the effective_chunk_index local entirely.
Passes chunk_index (may be None) directly into both ChunkContext and
_insert_position_markers, in both the Qdrant fast path and the
doc-text fallback.
- Fast path: when chunk_index is None and chunk_text was retrieved via
the offset lookup (notes/cards), skip the adjacent-chunk fetch.
Index arithmetic from a default 0 would query the chunks at positions
-1 and 1 even when the actual chunk is, say, 5/20 — silently producing
wrong "before"/"after" text. Mark both sides as truncated instead.
- Drop the now-redundant logger.warning in the doc-text fallback (the
response correctly communicates the unknown state via chunk_index=None).
Both existing response builders (`api/visualization.py:657` and
`auth/viz_routes.py:717`) already serialise `chunk_context.chunk_index`
unconditionally; `None` becomes JSON `null`. No route changes needed.
Adds 5 regression tests:
- ChunkContext.chunk_index propagates as None in fast path
- ChunkContext.chunk_index propagates as None in doc-text fallback
- Fast path with chunk_index renders "Chunk N of M" correctly
- _insert_position_markers renders "?/N" for None chunk_index
- _insert_position_markers renders explicit index when supplied
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Drop chunk_bbox_page from Qdrant payload — viz endpoints never read it
(page_number is the canonical PDF page field).
- Bump upsert BATCH_SIZE 10 → 100 now that payloads no longer carry PNGs.
- compute_chunk_bboxes_batch: move doc.close() into finally, replace
unused stored_page_num with _.
- purge_page_images.py: switch to anyio.run() per project convention,
and wrap AsyncQdrantClient in try/finally so the aiohttp session is
always closed (the class doesn't implement async-context-manager).
- Decorate new bbox unit tests with @pytest.mark.unit so they run under
the fast-feedback selector.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Latest reviewer comment flagged five items on top of the original PR. This
commit addresses every one:
🟡 1. Skip the offset-based Qdrant fallback for `doc_type=file` when
`chunk_index` is supplied. Qdrant Cloud's strict mode rejects unindexed
filter fields with HTTP 400, which `_get_chunk_from_qdrant` catches and
logs at `logger.error` — masking real Qdrant problems in monitoring.
Notes/cards keep the offset fallback (cheap, useful for legacy data).
🟡 2. Add a `logger.warning` and clarifying inline comment in the doc-text
fallback path when `chunk_index` is None — surfaces the pre-existing
"0/N misreport" so callers can detect it. Type-nullability propagation
is deferred to a follow-up (out of scope for this hotfix).
🟢 3. Simplify `if chunk_text and doc_id_int is not None:` →
`if chunk_text:` with an inner `assert doc_id_int is not None` for
`ty` narrowing. The outer second clause was dead.
🟢 4. Add `doc_type` `FieldCondition` to the offset-based image lookup in
both `visualization.py` and `viz_routes.py` for parity with the
`chunk_index` branches.
🟢 5. Inline the `chunk_filter` local in `visualization.py` directly into
the `must=[]` list (matches `viz_routes.py` style).
Adds `tests/unit/test_chunk_context_offset_gate.py` with three regression
tests covering the gate matrix: (file, with-index → skip offset),
(note, with-index → still tries offset), (file, no-index → still tries
offset). Lives at top-level rather than `tests/unit/search/` to side-step
a pre-existing circular-init issue in `nextcloud_mcp_server.search`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-chunk PDF page renders (~150–700 KB base64 PNG each) were the dominant
disk consumer in production, repeatedly tripping `No space left on device:
WAL buffer size exceeds available disk space` on welcomed-malamute Qdrant.
Replace the inline highlighted_page_image / highlighted_page_number /
highlight_count fields with a small `chunk_bbox` field:
list[(x0, y0, x1, y1)] of normalized [0, 1] floats, ~32 bytes per chunk.
Astrolabe (the only known consumer) renders the highlight client-side as
a percentage-positioned overlay on top of the existing /api/v1/pdf-preview
render-on-demand path (cbcoutinho/astrolabe#76).
- pdf_highlighter: new compute_chunk_bboxes_batch() that reuses the
existing _find_chunk_bbox text-search path, skipping all pixmap/PIL/PNG
work.
- processor: store chunk_bbox + chunk_bbox_page in the Qdrant payload,
drop highlighted_page_image + friends, drop the base64 import.
- visualization /api/v1/chunk-context and auth/viz_routes: read
chunk_bbox instead of highlighted_page_image.
- vector/__init__: stop eagerly re-exporting `processor`/`scanner` —
fixes a pre-existing circular import (search.algorithms ->
vector.placeholder -> vector/__init__ -> processor -> scanner ->
server.semantic -> search.bm25_hybrid -> search.algorithms partial).
Test suite that was broken on master (test_bm25_hybrid.py et al.) now
collects and passes.
- scripts/purge_page_images.py: ad-hoc, idempotent migration that
delete_payload's the legacy keys from existing points. No reindex
required; legacy chunks render the page with no overlay.
Pairs with cbcoutinho/astrolabe#76. Frontend handles missing chunk_bbox
gracefully, so this can land in either order.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add `doc_type` FieldCondition to the chunk_index-path highlighted-image
Qdrant filter in both `api/visualization.py` and `auth/viz_routes.py`,
matching the shape of `_get_chunk_by_index_from_qdrant`. Safe today (the
block is guarded by `doc_type == "file"` and Nextcloud file IDs are
globally unique) but prevents a latent bug if other doc types start
storing highlighted images.
- Demote `viz_routes.py` `ValueError` log from `error` to `warning` (lazy
%-style) — `_parse_int_param` raises on user-supplied bad input, which
is a 400 not a server error and shouldn't pollute error logs.
- Hoist `effective_chunk_index` to compute once at the top of
`get_chunk_with_context`, removing two duplicate assignments.
- Add `test_file_doc_type_qdrant_miss_yields_fast_404` to the management
endpoint tests, locking in the proxy-timeout fix contract.
- Add `tests/unit/test_viz_routes_chunk_context.py` mirroring management
coverage for the OAuth-session route: param forwarding (chunk_index /
total_chunks), `doc_type=file` fast 404, and 400 on invalid int params.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Replace bare int() casts for start/end/context_chars in chunk_context_endpoint
with _parse_int_param, matching visualization.py bounds (0–10M for offsets,
0–10K for context_chars), and add the missing end > start guard.
- Initialize page_number from chunk_context.page_number so non-file doc_types
surface it; include page_number, chunk_index, and total_chunks unconditionally
in the response. Only highlighted_page_image stays gated on its own truthiness.
- Add a chunk_index forwarding regression test that asserts the new kwargs reach
get_chunk_with_context and appear in the response payload.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #767 review noted that the OAuth viz route used bare int() parsing for
chunk_index and total_chunks while the bearer-token visualization route
validates them via _parse_int_param. Mirror the same bounds check so
total_chunks=0 and negative chunk_index return 400 instead of silently
suppressing adjacent-chunk context.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- test_registry.py: stub `mistralai.client.Mistral` in
`test_registry_mistral_wins_over_ollama`, mirroring the sibling
picker test, so the test doesn't depend on the SDK accepting
arbitrary keys.
- openai.py: convert remaining f-string `logger.info(...)` calls to
lazy `%s` formatting, aligning with the pattern in mistral.py and
the repo's logging convention.
- test_mistral.py: add four tests covering the defensive RuntimeError
guards in `embed()` and `_embed_batch_request()` — empty
response.data, single null embedding, batch null embedding, and
count-mismatch.
- docs/configuration.md: add `AWS_ACCESS_KEY_ID` and
`AWS_SECRET_ACCESS_KEY` rows to the env-var reference table; they
were already mentioned in prose but missing from the table.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- _retry.py: replace `assert last_error is not None` with explicit
`if last_error is None: raise RuntimeError(...)` so the original
rate-limit error is preserved under `python -O`.
- openai.py: drop the `_retry_factory` alias chain; rename the bound
decorator to `_retry_429` to match the pattern in mistral.py.
- mistral.py: comment the imports so future reviewers understand why
`from mistralai.client import …` is the canonical path on 2.x (no
top-level `__init__.py`; no `mistralai.models` subpackage either).
- docs/configuration.md: add `OPENAI_GENERATION_MODEL` and
`OLLAMA_GENERATION_MODEL` rows to the env-var reference table.
- test_mistral.py: add direct unit test for the `_is_rate_limit`
predicate (429 → True, 500 → False, missing-attr → False).
- test_registry.py: stub `mistralai.client.Mistral` in the registry
picker test, mirroring the Ollama sibling, so the test doesn't
depend on the SDK accepting arbitrary keys.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the Claude Code review on PR #772 plus the SonarCloud S1192 finding:
- Extract `retry_on_rate_limit` into `nextcloud_mcp_server/providers/_retry.py`
as a parametric decorator. OpenAI and Mistral now share the same backoff
loop; future providers can reuse it without copy-paste.
- New `tests/unit/providers/test_retry.py` covers the decorator: 429 retry +
success, non-429 immediate re-raise, MAX_RETRIES exhaustion, default
predicate, and unrelated exception passthrough.
- Tighten Mistral SDK import to `from mistralai.client.errors import SDKError`
(the canonical sub-path; the reviewer's `from mistralai.models import
SDKError` does not exist in mistralai 2.4.5).
- Replace `MistralProvider.close()`'s direct `__aexit__` call with a no-op +
comment — the Speakeasy-generated client has no public close hook and the
underlying httpx client is closed by GC.
- Extract the duplicated "Embedding not supported" message to a module-level
constant (SonarCloud S1192).
- Align `Settings.get_embedding_model_name()` Bedrock check with the registry
by also considering `bedrock_generation_model`.
- Add the `mock_mistral_client` fixture to
`test_mistral_no_embeddings_disabled` for parity with the rest of the file.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a hosted Mistral embedding option (mistral-embed, 1024-dim) alongside
the existing Bedrock / OpenAI / Ollama / Simple providers. Implementation
mirrors OpenAIProvider: lazy dimension detection with a known-models lookup,
chunked batch requests, defensive index sort, and a 429-aware retry decorator.
In the same change, ProviderRegistry switches from os.getenv to the
dynaconf-backed Settings dataclass so all five providers share a single
configuration path. config.py gains the previously-uncovered Bedrock keys,
the new Mistral keys, the missing OPENAI_GENERATION_MODEL /
OLLAMA_GENERATION_MODEL, and SIMPLE_EMBEDDING_DIMENSION.
Auto-detection priority: Bedrock → OpenAI → Mistral → Ollama → Simple.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a search tool that finds contacts by free-text substring match
across the four fields users actually look at:
- full name (FN)
- nickname
- any email address
- any phone number (compared as digits-only so "+1 234-567-890"
matches a search for "2345678")
Without this tool, an MCP client that wants to "find John's email"
has to pull every contact via list_contacts and filter client-side,
which costs a CardDAV REPORT per addressbook even when the user only
has a single hit. Doing the filter server-side (still cheap — it
streams the full vcards but discards non-matches before serialising
the response) keeps the tool surface symmetric with the rest of the
contacts API: list, get, create, update, delete, *search*.
When ``addressbook`` is omitted the search spans every addressbook
the authenticated user can read.
License: AGPL-3.0, matching the project.
Two related bugs surfaced in production while viewing chunks from the
Astrolabe frontend on the AWS-hosted MCP server:
1. PyMuPDF document closed: in _fetch_document_text the fallback path
referenced pdf_doc.page_count after pdf_doc.close(), raising
"document closed" and returning None. The slow PDF re-parse already
completed but its result was discarded. Capture page_count into a
local before close().
2. Slow/fragile chunk lookup: get_chunk_with_context filtered Qdrant by
(chunk_start_offset, chunk_end_offset). Those fields are not part of
the always-indexed payload schema, and with strict_mode enabled they
yield 400 errors. Even with manually-added indexes the filter is
fragile if a doc is re-chunked. Switch to chunk_index (always
indexed) as the primary lookup key, falling back to offset-based
lookup when callers don't supply it.
Plumb chunk_index/total_chunks through both the management API
(api/visualization.py) and the OAuth viz route (auth/viz_routes.py).
Apply the same change to the highlighted-image lookup so all four
chunk-context Qdrant queries prefer the indexed field.
Skip the slow PDF re-parse fallback entirely for files: when both the
chunk_index and offset Qdrant lookups miss, re-downloading and
re-parsing the source PDF won't find the chunk either, and routinely
exceeds 30s on large documents - which is the proxy timeout in
Astrolabe. Notes/cards keep the document-fetch fallback (cheap).
Removes dead code (_get_file_path_from_qdrant) that was only used by
the now-unreachable file fallback path.
Companion change in the Astrolabe app passes chunk_index from search
results through to the new endpoint params.
---
_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>
The default property set in `search_files` omits `<oc:fileid>`, so
`find_by_type` returned descendant dicts with no `file_id` — which
`NextcloudClient.find_files_by_tag` then silently dropped via its
dedup-by-id guard. Net effect: tag-on-folder produced zero expanded
descendants in CI (single-user / nc31, nc32). Mirrors the explicit
property list already used in `WebDAVClient.find_by_tag`.
Also addresses three nits from the PR #765 bot review:
- trim multi-paragraph docstring on `_normalise_search_result`
- trim multi-line docstring on `find_files_by_tag`
- match `is not None` ID-extraction pattern in the descendant loop
- assert positional `mime_type` arg in the unit test
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
NextcloudClient.find_files_by_tag now mirrors the directory semantics
already used by the exclusion path (issue #710): when a tagged item is
a folder, walk its descendants via WebDAV SEARCH (Depth: infinity) and
include any files matching the MIME filter. Without this, tagging the
root of a corpus with `vector-index` indexed nothing because the tag
applies to the directory only, not to its children.
The vector scanner additionally consults EXCLUDED_TAGS now, so a folder
marked off-limits is skipped even if it (or an ancestor) carries the
include tag — defense-in-depth, matching the "exclusion wins" contract
already enforced by the MCP file tools.
Also addressed a recurring memory-style nit: pre-existing f-string log
lines in find_files_by_tag were converted to lazy %-style.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
