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>
Defer publication of `_qdrant_client` until after the in-lock backfill +
payload-index migration awaits complete. The fast-path check at the top of
`get_qdrant_client` reads the singleton without holding the init lock, so
publishing the constructed-but-unmigrated client let concurrent fast-path
callers fire filtered searches before `_ensure_payload_indexes` ran —
producing HTTP 400 ("Index required but not found") on Qdrant Cloud strict
mode. Local `provisional` is now used for every await inside the lock; the
global is assigned exactly once, last.
Replace the five hand-rolled `scroll(..., limit=10000)` calls in
`vector/scanner.py` (notes / files / news / deck-cards deletion tracking,
plus the timestamp scroll) with a single paginated `_scroll_all_points`
helper. The previous single-page cap silently dropped deletion-tracking
points beyond the first 10 k for any user past that threshold. Pagination
follows Qdrant's documented contract (loop until `next_page_offset is
None`) with a fixed per-page `_DELETION_TRACKING_PAGE_SIZE = 1024`.
Extract `_create_one_payload_index` from `_ensure_payload_indexes` to drop
its cognitive complexity below the SonarQube limit (17 → ≤ 15) without
losing the per-field error-containment rationale; every comment is
preserved verbatim on the helper.
Drop the stale `SearchResult.id` `int | str` comment and the redundant
`str(d)` coercion in `_verify_news_items` — the contract has been
str-only since the producer-side stringification landed earlier in this
PR.
Fix eight `doc_id=<int>` test calls in `test_chunk_context_offset_gate.py`
that violated the `doc_id: str` signature of `get_chunk_with_context`,
plus align `_make_result` in `test_verification.py` to coerce `id=str(...)`
matching the production contract — and update 30+ assertions from int
sets (`{1, 2, 3}`) to str sets (`{"1", "2", "3"}`) so the tests now model
the post-PR `SearchResult.id: str` reality end-to-end. Previously these
were masked by the `str(d)` coercion now removed from production.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round-14 review surfaced one blocking and one important issue.
context.py: the comment justifying `skip_offset_lookup` claimed
chunk_start/end_offset weren't in _PAYLOAD_INDEX_FIELDS — round 13
indexed both as INTEGER, so the comment now actively misleads. Replace
with the real reason: an indexed chunk_index miss is canonical (both
paths hit the same Qdrant collection), and skipping the offset filter
avoids a redundant round-trip.
verification.py: hoist an is_valid_nextcloud_doc_id guard before the
`int(d)` cast in _verify_news_items, mirroring the boundary-validation
pattern already in _fetch_document_text. Coerce via `str(d)` because
SearchResult.id is `int | str` (D1 forward-compat widening). Malformed
ids now surface as a logger.warning rather than a generic debug line;
fail-open semantics are preserved.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add chunk_start_offset / chunk_end_offset to _PAYLOAD_INDEX_FIELDS so
the legacy offset-based fallback in search/context.py works on Qdrant
Cloud strict mode (pre-#75 clients have no chunk_index payload).
- Cover chunk_index / chunk_start_offset / chunk_end_offset in the
payload-index summary test; refresh the stale field-list comment.
- Flag the is_valid_nextcloud_doc_id gate at both chunk-context handler
sites with a TODO for future non-numeric doc_types.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- _group_int_doc_ids: use type(value) is not int instead of isinstance,
since bool is an int subclass and would otherwise stringify to
"True"/"False" and corrupt legacy payloads on backfill.
- Replace doc_id.isdigit() guards in 5 boundary sites
(api/visualization, auth/viz_routes, search/context note/news_item/
deck_card branches) with a shared is_valid_nextcloud_doc_id helper
that rejects "0", leading zeros, and Unicode digit classes
(superscripts, Arabic-Indic, Devanagari) which pass isdigit() but
cannot be valid MySQL AUTO_INCREMENT IDs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- search/context.py: drop the doc_type=='file' guard on skip_offset_lookup
so notes / deck cards / news items also bypass the unindexed offset
fallback when chunk_index is available. Legacy chunk_index=None data
still uses the offset path.
- vector/qdrant_client.py: clarify the backfill/_ensure_payload_indexes
ordering invariant (backfill rewrites payload values only, never schema
or indexes). Acknowledge OSS-vs-Cloud uncertainty in the 400-branch
comment and the new-collection call-site comment.
- vector/scanner.py: hoist qdrant_client to function scope so the
file-scroll block doesn't depend on a name bound inside the
notes-scroll block.
- tests/unit/test_chunk_context_offset_gate.py: flip the note-with-
chunk_index test to assert the offset fallback is skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three coordinated fixes flagged as Important in the round-10 review of
PR #773:
1. Index chunk_index. The chunk-context fast path in
_get_chunk_by_index_from_qdrant and get_chunk_bbox_and_page_from_qdrant
filters on chunk_index, but the field was absent from
_PAYLOAD_INDEX_FIELDS. On Qdrant Cloud strict mode every chunk-context
lookup via chunk_index would 400 and silently fall back to the
document re-fetch path — the exact failure mode the chunk_index
shortcut exists to avoid. Added as INTEGER schema.
2. Catch raw network errors in _ensure_payload_indexes. The
create_payload_index loop only caught UnexpectedResponse, so an
httpx.ConnectError or asyncio.TimeoutError mid-loop would propagate
uncaught — leaving _qdrant_client assigned and silently skipping all
remaining fields. Added a broad Exception catch with the same
per-field containment as the 5xx path: log at ERROR with exc_info,
append to failed_fields, continue. New test covers the path.
3. Lazy-initialise _qdrant_init_lock. Constructing anyio.Lock() at
module import time works for the asyncio backend but anyio's docs
advise instantiating synchronization primitives within an async
context, and pyproject.toml's anyio_mode = "auto" means tests can
run under trio. Moved the construction into get_qdrant_client; safe
under cooperative multitasking because there is no await between the
None-check and the assignment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add an .isdigit() guard at the top of both chunk-context handlers so a
non-numeric doc_id fails fast with a clear 400 ("doc_id must be numeric,
got 'abc'") rather than silently bottoming out as a 404 from deep inside
get_chunk_with_context. The earlier int(doc_id) coercion was removed when
doc_id became a pure pass-through to Qdrant's keyword payload index, which
also dropped this boundary validation.
Also align test_backfill_emits_progress_log_every_20_batches' scroll stub
with real Qdrant: next_offset is now "next-1" (str) instead of 1 (int),
matching the sibling test_backfill_rewrites_int_doc_ids_to_str. Pure
stub-fidelity fix; production code already treats next_offset as opaque.
Addresses both 🟡 Important items from PR #773 review round 10.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the four 🟡 important findings from claude-bot review on PR #773:
str (non-Optional) and the guard would silently skip the Qdrant lookup
for an empty string. Removing the guard matches the type signature.
(`all([…, doc_id, …])` rejects None and empty string, plus
`assert doc_id is not None`). No code change needed.
`get_qdrant_client()` with a module-level `anyio.Lock`. Double-checked
locking keeps the steady-state hot path lock-free. Without this,
parallel cold-start callers could all enter the init block and run
`_backfill_doc_id_to_string` + `_ensure_payload_indexes` redundantly
(idempotent, but noisy). Pattern matches `auth/storage.py:2071`.
behavior with three tests covering the float-warning path (the gap
called out in the review), the str/None silent-skip paths, and the
int-grouping happy path.
Verification:
- ruff check / format: clean
- ty check -- nextcloud_mcp_server: clean
- uv run pytest tests/unit/: 969 passed
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two unrelated CI failures on this branch, one fix each:
- tests/integration/test_deck_vector_search.py: pass str(card.id) to
get_chunk_with_context. The function's contract is doc_id: str
(keyword-indexed in Qdrant), and real callers (viz_routes.py URL
path, server/semantic.py via str(result.id)) all stringify. The
test was the only int caller, hitting the .isdigit() guard added
earlier on this branch.
- tests/server/login_flow/test_login_flow_integration.py:
test_check_status_provisioned now accepts scopes=None as valid.
Per ProvisionStatusResponse in models/auth.py, None is the
documented sentinel for "all scopes granted" — and the web
provisioning path (provision_routes.py, used by Astrolabe's
"Enable Semantic Search" flow exercised by the new regression test
added on this branch) stores exactly that. The previous
is-not-None assertion hid behind test order until that flow ran.
- Replace anyio.sleep(0) with anyio.lowlevel.checkpoint()
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Skip and warn instead of stringifying floats / unexpected types in the
backfill helper. A stray doc_id=3.0 would otherwise be rewritten to
"3.0", which producers (str(int)) and the keyword index would never
match, and which int() on the verification side would reject. Also add
a doc_id=0 case to the backfill test to guard against a future
falsy-skip regression.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add the missing end-to-end coverage for the seam that PR #773's
`ALLOWED_MGMT_CLIENT` ↔ `astrolabeMcpClientOAuth00000000000` drift
bug exposed: browser → Astrolabe (NC PHP app) → MCP server's
management API on the `mcp-login-flow` profile. Every existing
`tests/integration/test_astrolabe_*.py` is marked `multi_user_basic`
and exercises the BasicAuth flow, not Login Flow v2 / OAuth.
The new test mirrors the production-shaped flow exactly:
1. Log in as admin via Playwright.
2. Navigate to `/settings/user/astrolabe`.
3. Click the "Enable Semantic Search" OAuth link rendered by
`oauth-required.php`. (Same selector Astrolabe's own e2e helper
uses — `third_party/astrolabe/tests/e2e/helpers/authorize.ts`.)
4. Click "Allow" on the Nextcloud OIDC consent screen.
5. Wait for the redirect back to the Astrolabe settings page.
6. Assert the "Enable Semantic Search" link is no longer visible.
Step 6 is the canary for the drift class: if Astrolabe's management
API call to `/api/v1/users/{id}/session` is rejected (HTTP 401, the
original bug), the session lookup falls back to "no token" and the
same `oauth-required.php` template re-renders with the link still
present — so the test fails loudly with a message naming the
likely cause.
Reuses `login_to_nextcloud` and `navigate_to_astrolabe_settings`
helpers from `tests/integration/test_astrolabe_multi_user_background_sync.py`
(already pattern-imported by the Plotly viz test). No fixture-level
OIDC client creation: `app-hooks/before-starting/26-configure-astrolabe-oauth.sh`
already provisions `astrolabeMcpClientOAuth00000000000` with the
correct redirect URI and scopes when `MCP_SERVER_URL` is set in the
shell that runs `docker compose --profile login-flow up`.
The test skips cleanly when admin is already authorized (typical
state on a re-run against a long-lived dev stack), so it's safe to
run repeatedly without manual reset.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🔴 Blocking finding from PR #773 latest review:
`get_chunk_bbox_and_page_from_qdrant` (`search/context.py:199`) still
declared `doc_id: int | str` and passed the raw value into
`MatchValue(value=doc_id)` at lines 239 and 256 without `str()`
coercion. After this branch's startup backfill normalises every Qdrant
`doc_id` payload to a string, an `int` filter would silently match zero
points — the function would return `(None, None)` instead of the
chunk bbox / page, and PDF highlight overlays would fail in production.
Take option 2 from the reviewer's two suggestions (annotation
tightening over inline coercion): the producer side of this PR has
already narrowed every other `doc_id` annotation to `str`, so this
function is the last hold-out. Pushing the contract into the type
system means `ty` will catch any future regression at the call site.
Production callers in `api/visualization.py` and `auth/viz_routes.py`
already pass `doc_id` (str) verbatim after the recent merge with
master's chunk_index-first refactor, so no caller-side changes needed.
Update the 9 calls in `tests/unit/test_chunk_bbox_helper.py` to use
string literals (`"42"` / `"99"` / `"1"`) instead of integers. The
mock doesn't validate `MatchValue` value types, so the tests passed
with stale int doc_ids today — but they were exercising a path
production no longer takes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit moved `mcp-login-flow`'s `ALLOWED_MGMT_CLIENT` to
`astrolabeMcpClientOAuth00000000000` so production-shaped Astrolabe
traffic actually validates. Update the management API test fixture to
match: the static OIDC client created in
`tests/server/login_flow/conftest.py:login_flow_static_client_credentials`
now uses the same id `app-hooks/before-starting/26-configure-astrolabe-oauth.sh`
provisions in real deployments, so the test path exercises the same
code as production rather than a substituted fixture-only id.
`mcp-multi-user-basic`'s allowlist is unchanged
(`nextcloudMcpServerUIPublicClient`) and the shared
`configure_astrolabe_for_mcp_server` fixture in `tests/conftest.py`
keeps that as its default, so multi-user-basic tests are unaffected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `mcp-login-flow` profile's `ALLOWED_MGMT_CLIENT` was set to the
test-fixture id `nextcloudMcpServerUIPublicClient` only, but the actual
Astrolabe app provisions its OIDC client as
`astrolabeMcpClientOAuth00000000000` (see
`app-hooks/before-starting/26-configure-astrolabe-oauth.sh:39`). All
tokens issued through the "Enable Semantic Search" flow were rejected
with HTTP 401 by `unified_verifier.py:222-227`'s allowlist check, and
the Astrolabe UI's retry loop subsequently exhausted the
`api/passwords.py` 5/hr rate limit (HTTP 429).
Switch the `mcp-login-flow` allowlist to Astrolabe's client id so
production-shaped traffic actually validates. The `mcp-multi-user-basic`
profile keeps `nextcloudMcpServerUIPublicClient` for the
`configure_astrolabe_for_mcp_server` test fixture.
Also bump `third_party/astrolabe` 0.13.12 → 0.14.0 to pull in the
chunk-context indexed-lookup fix (#75) and the PDF bbox highlight
overlay (#76) that match the master-side changes already merged on
this branch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer findings (1 blocking + 2 important):
- 🔴 Replace `import asyncio` / `await asyncio.sleep(0)` with
`import anyio` / `await anyio.sleep(0)` in the four async side-effect
helpers (_scroll_raises, _upsert_raises, _get_collection_raises,
_create_index). CLAUDE.md mandates anyio for all async operations;
conftest pins the backend to asyncio so the asyncio.sleep call worked
today, but the inconsistency would surface the moment that pin moves.
- 🟡 Replace the sentinel's zero dense vector with a single non-zero
element (`[1e-9] + [0.0] * (dimension - 1)`). Cosine distance is
mathematically undefined for the zero vector and Qdrant Cloud strict
mode rejects zero-vector upserts. The exact value doesn't matter
(sentinel never participates in a search — no user_id/doc_id/doc_type
payload) but the upsert itself must be valid.
- 🟡 Avoid the duplicate `get_collection` round-trip on every restart.
`_ensure_payload_indexes` now accepts an optional
`existing_schema: dict | None` parameter; when None it fetches
collection_info itself (and the get_collection-failure swallow still
applies), but `get_qdrant_client` already fetches collection_info
for dimension validation in the existing-collection branch — pass
`collection_info.payload_schema or {}` through to skip the second
call. The new-collection branch passes `existing_schema={}`
explicitly since a freshly created collection has no payload schema.
The 🟡 deck_card iteration-fallback finding doesn't apply: the
`isdigit()` guard at context.py:612 returns early before either the
fast-path or the iteration fallback runs, so non-numeric doc_ids
cannot reach the inner `c.id == int(doc_id)` comparison.
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>
Reviewer feedback (2 items):
- Add a BOOL payload index for `is_placeholder` alongside the three
KEYWORD fields. Strict-mode index-required filtering on Qdrant Cloud
enforces a payload index on any field used in a `FieldCondition`
regardless of value type, so `get_placeholder_filter` and
`delete_placeholder_point` would have produced HTTP 400 on Cloud
instances even after this PR's KEYWORD fix.
Implementation: replace `_KEYWORD_PAYLOAD_FIELDS: tuple` with
`_PAYLOAD_INDEX_FIELDS: dict[str, PayloadSchemaType]` so each
field carries its own schema type. Rename
`_ensure_keyword_payload_indexes` to `_ensure_payload_indexes` since
the function now creates more than just KEYWORD indexes. The
per-field log line now includes the schema type
("Created KEYWORD payload index on 'doc_id'", "Created BOOL payload
index on 'is_placeholder'") so operators can tell which type was
created without checking the source.
- Correct the misleading `wait=True` docstring in
`_apply_backfill_writes`. The previous wording said
`_ensure_payload_indexes` runs "immediately after this function",
but `_apply_backfill_writes` is called in a loop inside
`_backfill_doc_id_to_string` — the index creation runs after the
backfill function *returns*, not after each write. Rewrote the
docstring to capture both load-bearing reasons:
(1) per-batch commit ordering for crash-recovery safety, and
(2) ensuring the keyword index built later covers committed
payloads only.
Adds `test_ensure_payload_indexes_includes_is_placeholder_as_bool`
asserting the schema type is BOOL specifically. Existing tests
updated to use the new dict-based registry (side_effect lists now
extend to all four entries; field-set assertions derive from the
registry instead of hardcoding 3 KEYWORD names).
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>
Reviewer feedback (3 important + 3 nits):
- Wrap _ensure_keyword_payload_indexes' get_collection() call in
try/except. The qdrant_client singleton is already assigned by the
time this function runs, so a transient timeout/DNS failure
propagating out left the process holding a usable client with the
migration silently skipped on every subsequent call. Now logs ERROR
with exc_info and returns; next process restart retries.
- Add `and "doc_id" in point.payload` guard to the four set
comprehensions in scanner.py (indexed_doc_ids, indexed_file_ids,
indexed_item_ids, indexed_card_ids). Previously a payload missing
the doc_id key would raise KeyError and crash the entire scan.
- Tighten test_ensure_keyword_payload_indexes_logs_400_as_warning to
match the per-field warning prefix exactly (`startswith("Schema
conflict on payload index")`), so a future change adding 400s to
the partial-failure summary surfaces here as a count mismatch.
- Add new-collection vs existing-collection context to the
_backfill_doc_id_to_string docstring's `dimension` parameter.
- Replace the misleading "rewrote 0/N from int to str" wording when
no rewriting was needed with "N points scanned, none required
rewriting (collection already in str form)".
- Add test_ensure_keyword_payload_indexes_logs_and_returns_when_
get_collection_raises mirroring the scroll-failure test.
SonarCloud (1 CRITICAL + 1 MINOR):
- Refactor _backfill_doc_id_to_string to bring cognitive complexity
under 15 (was 19). Extracted two pure helpers: _group_int_doc_ids
(group point IDs by stringified doc_id) and _apply_backfill_writes
(apply set_payload calls and return rewritten count). The main
function's scroll/loop/sentinel structure is unchanged.
- Add `await asyncio.sleep(0)` to the three async test side_effect
helpers (_scroll_raises, _upsert_raises, _create_index) so they use
an actual async feature (S7503). The async-callable shape is still
required to avoid the AsyncMock unawaited-coroutine warning when
side_effect raises.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses three important findings from the latest reviewer comment:
- Add progress INFO log every 20 scroll batches (≈5120 points at
batch_size=256) in _backfill_doc_id_to_string so a long-running
migration on a large collection (50k+ points) doesn't look like a
startup hang. The line carries collection name, scanned count, and
rewritten count so it doubles as a heartbeat.
- Track non-400 failures in _ensure_keyword_payload_indexes and emit
a WARNING summary line listing every field that failed to get an
index. Per-field ERROR lines are easy to miss in startup noise; the
summary makes the partial-failure state visible at a glance.
- Split the sentinel upsert out of the data-scroll try/except in
_backfill_doc_id_to_string. A scroll-time failure still logs ERROR
with the new "scroll failed" wording (data is incomplete). A
sentinel-write failure now logs WARNING with "data succeeded but
sentinel write failed" wording — data is correct, only the
short-circuit marker is missing, and the next restart re-scrolls
an already-clean collection (idempotent zero-write) before retrying
the upsert.
Also fix the RuntimeWarning emitted by
test_backfill_logs_and_returns_when_scroll_raises: replace the bare
`RuntimeError` side_effect with an async-callable side_effect so
AsyncMock awaits the coroutine before the exception propagates.
Three new unit tests cover the new branches:
test_backfill_emits_progress_log_every_20_batches,
test_backfill_logs_warning_when_sentinel_upsert_fails,
test_ensure_keyword_payload_indexes_summarises_failed_fields.
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>
- Remove three stale `# Use numeric file ID` / `# Pass file path` comments
in scanner.py. file_id is already normalized to str() above each call
site, so the inline comments mislead readers.
- Wrap `_backfill_doc_id_to_string` scroll loop + sentinel upsert in
try/except Exception. The qdrant_client singleton is assigned before
this migration runs, so a transient scroll failure was leaving the
process holding a usable client with int payloads permanently
unbackfilled until the next restart. Catch broadly, log ERROR with
exc_info, and return without writing the sentinel — next process
restart retries from scratch.
- Note `:memory:` mode behavior near the sentinel constants so future
readers don't read the every-start scroll as a bug.
- Document the two degraded-migration ERROR log signals in
docs/configuration.md so operators know when a clean restart is
required to recover indexing.
- Add unit test asserting scroll-time exceptions are logged and swallowed
without writing the sentinel.
Closes round-4 review feedback on PR #773.
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>
- Add a fixed-UUID sentinel point written after a successful doc_id
backfill so subsequent restarts retrieve it and short-circuit the
O(N) scroll. Sentinel has no user_id/doc_id/doc_type payload so
production search filters never see it.
- Pre-fetch payload_schema in _ensure_keyword_payload_indexes and
silently skip fields that are already indexed; the "Created KEYWORD
payload index" INFO log fires only on actual creation.
- Narrow stale `int | str` doc_id annotations to `str` across
search/verification.py (BatchVerifier return type, per-verifier
accessible sets, by_type / accessible_by_type / inaccessible
collections); drop the now-redundant `type(d).__name__` prefix in
the dropped-docs log.
- Align the backfill log message with the PR description's
"Running doc_id backfill" promise; add a caller cross-reference to
the wait=True comment.
- Fix _get_file_path_from_qdrant docstring (file_id is str, not numeric).
- Convert legacy `id=1` to `id="1"` in test_search_result.py to match
the SearchResult.id: str annotation.
Three new unit tests cover sentinel-found, sentinel-written, and
skip-existing-index branches; existing backfill tests pass dimension
and explicit retrieve.return_value=[] for the no-sentinel path.
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>
- _ensure_keyword_payload_indexes: distinguish 400 (schema conflict, warning)
from other status codes (5xx/network, error) so a transient outage doesn't
silently leave the collection unindexed.
- build_search_result_from_point: use .get("doc_id") + return None on missing
instead of KeyError-crashing the search; reverse metadata merge order so
payload-derived chunk_index/total_chunks win over caller-supplied extras.
- docs/configuration.md: restore the OpenAI/Mistral/Bedrock/Simple provider
sections + reference-table rows that were dropped in the rebase. Reword
the "Startup migrations" bullet to describe what the code actually does
(no sampling — full scroll, zero writes when clean). Add operator note
about the SemanticSearchResult.id TypeError path.
- tests: pytest.approx for float equality (Sonar python:S1244); coverage
for non-400 → ERROR, payload={doc_id: None}, and missing doc_id key.
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>
Addresses reviewer feedback on PR #773:
- Backfill set_payload now uses wait=True to avoid a race where
_ensure_keyword_payload_indexes builds the KEYWORD index before
fire-and-forget writes have committed, leaving int payloads
invisible to filters.
- Batch points sharing the same int doc_id into a single set_payload
call (one document → many chunks → one round-trip instead of N).
- Drop _has_int_doc_id_sample short-circuit. The sample's false-negative
window (clean first 256 results, ints further in) is gone; full scroll
is the dominant cost on first run anyway.
- Simplify _ensure_keyword_payload_indexes: the "already exists" 400
branch was dead code (Qdrant returns 200 on identical re-create); any
400 now logs a warning and continues.
- search/context.py: comment the broadened file-type guard. Add explicit
not doc_id.isdigit() checks at the top of note/news_item/deck_card
branches in _fetch_document_text so malformed payloads surface as
warnings instead of being swallowed by the broad except.
Also extracts build_search_result_from_point into search/algorithms.py
to deduplicate the 71-line payload-extraction loop shared by
SemanticSearchAlgorithm and BM25HybridSearchAlgorithm. This fixes
SonarQube's quality-gate failure (4.0% new-code duplication, max 3%).
Test coverage:
- 7 new unit tests for build_search_result_from_point covering missing
payload, note/file/deck_card metadata, int doc_id coercion, and
metadata_extras merging.
- Replace _has_int_doc_id_sample tests with clean-collection no-op and
per-batch grouping tests.
- Update set_payload assertions from wait=False to wait=True.
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>
