Address PR #935 round-3 review:
- search/verification.py: rewrite _verify_mail_messages to batch by mailbox.
get_message triggers a server-side IMAP body fetch, so per-result verify
issued one IMAP FETCH per hit; now it calls the DB-cached list_messages once
per mailbox (mailbox_id comes from the Qdrant payload via result.metadata)
and intersects — O(unique mailboxes) light calls instead of O(results) IMAP.
- vector/mail_content.py: include Cc/Bcc in the indexed text so recipient
queries match; move MAIL_SCAN_MAX_PER_MAILBOX here (shared by scanner index
window + verifier presence window) with a note that it equals the Mail OCS
per-request max (100), so it's a fixed constant not a config knob.
- client/mail.py: clamp list_messages limit to 1..100 at the client layer.
- tests: add test_mail_content.py (exact-layout contract for build_mail_content);
rewrite the mail verifier tests for the batch-per-mailbox shape.
Left as-is: ValidationError isn't caught in the list-endpoint tools — consistent
with nc_notes_*/nc_deck_* and not a regression.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add read-only support for the Nextcloud Mail app, plus semantic indexing
of mail messages. The MCP server never speaks IMAP/POP3 itself: it calls
the Mail app's CSRF-free OCS API (/ocs/v2.php/apps/mail/api/...) with the
existing Basic-Auth app-password flow and an OCS-APIRequest header, and the
Mail app handles IMAP server-side.
- client/mail.py: MailClient (accounts, mailboxes, messages, message,
attachment), OCS-envelope aware.
- models/mail.py: Pydantic models with the API's camelCase aliases.
- server/mail.py: 5 read-only MCP tools (mail.read scope), registered in
AVAILABLE_APPS.
- Vector pipeline: new "mail_message" doc_type wired into scanner
(scan_mail_messages, newest-N per mailbox), processor (body -> markdown
embedding), per-id verifier, and context expansion.
- Tests: client API, model round-trips, verifier behavior; consent-backstop
test now derives its allowed set from INDEXED_DOC_TYPES.
- README + semantic-search docstrings updated.
Requires Mail 5.x / Nextcloud 32+ and a mail account configured in the
Mail app. Follow-up: astrolabe must advertise "mail_message" in its
enabled_doc_types capability for search under admin doc_type restriction.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address the two important findings from the claude bot's latest re-review:
- _verify_files: skip get_excluded_file_paths entirely when the tag REPORT
returns no files. An empty `tagged` yields an empty `tagged_ids` regardless
of exclusions, so the lookup's 2xlen(EXCLUDED_TAGS) WebDAV fan-out is wasted
work in the common "this tag matched nothing" case. The per-result loop still
runs, so malformed doc_ids are still kept (fail-open) — pinned by a new test
(test_verify_files_empty_tag_set_skips_exclusion_lookup), which also asserts
the exclusion lookup is never awaited.
- Rewrite the semaphore comment: it claimed "the slot bounds them", but the slot
only caps concurrent *searches* — get_excluded_file_paths internally spawns a
task group issuing 2xlen(EXCLUDED_TAGS) concurrent WebDAV calls, so live
Nextcloud connections can exceed VERIFICATION_CONCURRENCY. Comment now says so
and points at configuration.md.
The third 🟡 (sequential dir expansion in find_files_by_tag) is pre-existing and
flagged by the reviewer as a follow-up, not part of this PR.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve the latest PR-review comment on the verify-on-read tag-gate work:
- tests: stringify note IDs in test_verify_on_read.py so SearchResult.id
matches production (scanner stringifies all IDs on write) — helper and the
keeps/deleted/mixed/dedupe assertions (blocking).
- tests: make the unshared-file negative control a PDF so the drop is
unambiguously "unshared", not a mime_type_filter mismatch.
- config: add Validator("VECTOR_SYNC_PDF_TAG", len_min=1) — an empty tag name
would make find_files_by_tag("") misbehave in the verifier and scanner.
- verification: correct the _verify_files comment — two batch fetches (tag
REPORT + EXCLUDED_TAGS lookup) are held under one semaphore slot; the
pure-Python intersection runs outside it.
- tests: de-duplicate the minimal-PDF constant into a shared PDF_BYTES in
tests/integration/conftest.py, imported by both integration modules.
Verified: ruff/format/ty/unit all green; the two integration modules
(10 tests) pass against a local Nextcloud (app-only, no MCP profile needed).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Verify-on-read only checked file *accessibility* (file_accessible_by_id),
never tag membership, so a file removed from the `vector-index` tag (but
still readable) kept surfacing in semantic search, and stale points only
got evicted when they happened to rank in a search's top-K.
Rework `_verify_files` to gate on current `vector-index` tag membership via
a single batch `find_files_by_tag(tag, mime_type_filter="application/pdf")`
REPORT per search (plus a one-shot EXCLUDED_TAGS lookup for exclusion-wins
parity) — exactly what the scanner indexes. A file is kept iff it is in that
set, so untagged / deleted / excluded files drop out immediately and the
existing eviction wiring reclaims their Qdrant points. The gate is strict
for all file results, own and shared. Mirrors the batch-fetch-and-intersect
shape of `_verify_news_items` (one semaphore slot, fail-open on fetch error,
malformed-id keep).
- Promote the tag name to a `vector_sync_pdf_tag` Settings field (dynaconf
env mapping VECTOR_SYNC_PDF_TAG) used by both scanner and verifier;
drop the scanner's direct os.getenv.
- Expose `find_files_by_tag` on NextcloudClientProtocol.
- Rewrite the file-verifier unit tests (tagged/untagged/deleted/excluded/
fail-open/non-numeric); update the ACL + verify-on-read integration tests
to seed tagged PDFs.
- Amend ADR-019 and the configuration.md verify-on-read latency budget.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- 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>
- 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>
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>
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>
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 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>
Production was logging two cascading classes of Qdrant errors against the
welcomed-malamute deployment:
1. HTTP 400 — "Bad request: Index required but not found for \"doc_id\" of
one of the following types: [keyword]". The collection was created via
create_collection() with no payload indexes, so any FieldCondition
filter on doc_id failed at the Qdrant layer (placeholder writes/reads,
eviction, search context lookups).
2. Compounding the missing index, producers wrote a mix of int and str
doc_ids: webhook_parser stringified node_id, scanner stringified note
IDs, news IDs, and deck card IDs — but the file scanner passed the
numeric file_id through unchanged. A keyword index would not have
covered both kinds even if it had existed.
This change:
- Normalizes doc_id to str at every producer site (scanner.py:459,
DocumentTask.doc_id, indexed_*_ids reads from Qdrant).
- Tightens str|int annotations to str across placeholder.py,
eviction.py, search/verification.py, search/context.py,
SearchResult.id, and the auth/api visualization endpoints.
- Defensive str() coercion on doc_id reads in semantic.py /
bm25_hybrid.py / vector/visualization.py for the transition window
before the backfill runs.
- Adds an idempotent startup migration in get_qdrant_client():
- _ensure_keyword_payload_indexes creates KEYWORD indexes for
doc_id, user_id, and doc_type (tolerates "already exists" 400s).
- _backfill_doc_id_to_string scrolls the collection once and rewrites
int doc_ids to str. Skipped after a quick sample shows no legacy
int payloads.
- Public API preserved: SemanticSearchResult.id stays int via explicit
int(r.id) narrowing in server/semantic.py — surfaces a TypeError with
actionable context if a future doc_type ships non-numeric ids.
- Documents the startup migration in docs/configuration.md.
Tests: 11 new unit tests in tests/unit/vector/test_qdrant_client.py
covering happy path / already-exists / unrelated-400 for the index
helpers, and sample-skip / mixed-batch rewrite / payload=None edge cases
for the backfill. 889 unit tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six review items raised; four required code changes (#3, #4, #5, #6) and
two were resolved without code changes (#1 audit-only, #2 informational).
* search/verification.py — clarify the granularity asymmetry between the
whole-batch fail-open (structural API failure) and the per-item fail-open
(single bad stored doc_id). Future readers no longer need to derive why
the two paths have different blast radii from the code alone.
* models/semantic.py — `dropped_document_count` description now explicitly
notes that subtracting it from `verified_chunk_count` is not a meaningful
operation, since the two fields count different units (documents vs
chunks). Surfaces the unit mismatch where MCP clients actually see it.
* server/semantic.py — clarify the per-doc_type over-fetch comment so the
N×2 pre-merge Qdrant cost (vs the cross-app branch's 1×2) is explicit
rather than implied by "same 2× over-fetch budget".
* tests/unit/search/test_verification.py — add four new 429 unit tests
(notes/news/files/deck) mirroring the existing 5xx-keeps pattern. Locks
in that `_is_definitive_404_or_403` returns False for 429 so a future
refactor cannot accidentally treat rate-limit responses as permanent
revocations.
Audit confirmation for review item #1: all four `WebDAVClient.get_file_info`
call sites already handle the new `HTTPStatusError`-on-404 contract
(verification.py:156, tests/integration/test_rag.py:139,
tests/unit/client/test_webdav.py:153/190). No silent breakage internal to
this repo.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three minor fixes from the round-11 review on PR #750:
- bm25_hybrid.py:209 — Comment said `doc_id` is `int (notes) or str (files)`,
which is backwards. Notes, news_items, and deck_cards are stored as `str`
(scanner.py:241, 666, 867); files are stored as `int` (scanner.py:425).
Updated to point readers at scanner.py as the source of truth.
- verification.py:338 — Lowered the News-API 403/404 log line from `info`
to `debug`. The News app being uninstalled or disabled is a predictable
operational state (matching the other verifiers' debug-on-not-found
paths), so this should not generate operator-dashboard noise. Transient
errors immediately below stay at `warning` because they're unexpected.
- semantic.py:809 — `nc_get_vector_sync_status` was reading
`document_receive_stream` via `getattr(..., None)`, but the attribute is
guaranteed-defined on both `AppContext` and `OAuthAppContext` (as a
field with `None` default). The defensive `getattr` masked typos that
the eviction_task_group access at semantic.py:197-199 deliberately
surfaces. Switched to direct access; the `if … is None:` value-check
below is preserved (the attribute can legitimately be None before sync
starts).
Items deliberately deferred (with rationale in the plan file):
- News verifier semaphore-hold during get_items (reviewer: "not required
here, just worth tracking"; ADR already lists follow-ups).
- Hardcoded 2× over-fetch / VERIFICATION_OVERFETCH (TODO already in code).
- Integration test for the real Qdrant eviction filter (reviewer marked
low-priority; type-preservation chain is unit-tested).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Tighten verify_search_results signature: client: Any → NextcloudClientProtocol
- Collapse 3 copy-pasted lock-justification comments to a single-line pointer
- Add logger.debug timing around the verify_search_results call site
- Add logger.debug timing around the unbounded news.get_items fetch
- Rename SemanticSearchResponse.dropped_count → dropped_document_count to make
the chunks-vs-documents unit asymmetry explicit at the API boundary
- Drop unreachable duplicate 409 branch in WebDAVClient.move_resource
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add concurrency-safety comments to per-verifier accessible sets in
_verify_notes/_verify_files/_verify_deck_cards. Same rationale as
accessible_by_type in verify_search_results: anyio is cooperative,
set.add() is not an await point.
- Document 401 exclusion in _is_definitive_404_or_403 (treated as
transient because it usually signals expired credentials, not
permanent denial).
- Note multi-user compounding in the news verifier semaphore comment:
N concurrent users hold N slots out of the shared budget.
- Log inaccessible doc ids with a type tag (e.g. "int:42" vs "str:42")
so ghost-record logs disambiguate id types.
- Type the BatchVerifier alias and the four verifier function signatures
with NextcloudClientProtocol instead of Any (algorithms.py exposes
the right interface; the protocol is runtime_checkable).
- Surface verified_chunk_count vs dropped_count semantics in the
nc_semantic_search tool docstring Returns block (chunks vs unique
documents).
- Add comments to the two max_concurrent=20 sites in server/semantic.py
noting they are intentionally distinct from
settings.verification_concurrency (different request phases).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address findings surfaced by `pre-push-review` after the round 8 sweep:
- Add deck verifier symmetry tests (404, transient 5xx, unexpected
exception, non-numeric metadata) so deck has the same shape as the
notes/news/files verifiers. Also add unexpected-exception tests for
the news and file verifiers, which had `except Exception` branches
no test was reaching. Keeps the registry-style verifier coverage
uniform.
- Modernize sibling field types in `VectorSyncState`, `AppContext`,
and `OAuthAppContext` from `Optional[X]` to `X | None`, matching the
`eviction_task_group: TaskGroup | None` field added in the round 8
diff (resolves the inconsistency flagged by A6). The lone remaining
`Optional` import is dropped.
- Reverse cross-reference direction in the verifier docstrings: the
later-defined `_verify_deck_cards` and `_verify_news_items` now
point at `_verify_notes` as the canonical hoisted-cast pattern,
rather than `_verify_notes` forward-referring to verifiers defined
below it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Rename `verified_count` → `verified_chunk_count` to make the count
granularity explicit at the field name (chunks vs unique docs).
- News verifier now fails open *per-item* on non-numeric stored doc_ids
(matches notes/files/deck shape); a single bad id no longer rescues
definitively-missing siblings from eviction.
- Update note-verifier integration test to use string doc_ids end-to-end
to match production storage (scanner.py:241 stringifies note ids).
- Add regression test for the closed-task-group race guard in
`verify_search_results` so the RuntimeError swallow is locked in.
- Convert remaining f-string logger calls in `server/semantic.py` to
lazy %-style formatting (per repo convention).
- Document `evict_on_missing` as a developer/test flag (no env var) and
flag the `get_file_info` 404→raise contract change in its docstring.
- Add a TODO(ADR-019) breadcrumb for the hardcoded 2× over-fetch so
future tuning has a clear hook.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 7 raised 5 issues; this round addresses all of them and fixes
the underlying causes (not just the comments) where applicable so
they don't get re-flagged in future passes.
Critical:
- verified_count description in SemanticSearchResponse said "unique
documents" but the value is len(verified_results), a chunk count.
Description rewritten to accurately document chunk-level granularity
AND explicitly call out the asymmetry with dropped_count (which
counts unique (doc_id, doc_type) pairs).
- _verify_files false-eviction risk: the round-6 doc-only fix was
re-flagged. Address at the source — widen WebDAVClient.get_file_info
to raise HTTPStatusError on 404 (matching the rest of the client
convention) and reserve None for the genuinely ambiguous
malformed-PROPFIND case. _verify_files now keeps the result on None
(cannot tell whether the file exists) and evicts only on a
definitive HTTPStatusError 404. Tests updated; new test added for
the malformed-XML keep-result path.
Non-critical:
- News verifier semaphore lifetime now explicitly documented: one
slot held for one deduplicated fetch per search is the correct
backpressure behaviour.
- Cross-reference comments in _verify_notes / _verify_deck_cards no
longer claim "Mirrors X" pointing at functions defined later in
the file; now use direction-neutral "parallel implementation in".
- accessible_by_type is mutated by concurrent run_verifier tasks; a
comment explains why this is race-free under anyio's cooperative
multitasking (distinct keys per task, no await between read and
write) so a future reader doesn't add a redundant lock.
- Knock-on: tests/integration/test_rag.py wraps get_file_info in a
try/except for the new contract.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes out the remaining nits flagged in the round-6 review.
Critical:
- _verify_files contract comment now enumerates all None-return cases
(404 + malformed PROPFIND XML) and documents the false-eviction
trade-off; self-healing via re-indexing recovers
- int(r.id) cast at the SemanticSearchResult boundary now raises a
TypeError with explicit doc_type/value context instead of bubbling
up as an opaque "Search failed: ..." McpError
Design observations:
- nc_semantic_search_answer docstring documents the per-note
round-trip cost from the post-verification race guard
- News verification latency hint added to configuration.md
- SemanticSearchResponse exposes verified_count + dropped_count so
short result pages on high-ghost-density indexes are
distinguishable from genuine scarcity. verify_search_results now
returns (kept, dropped_count); production caller and tests updated
Minor:
- Comment clarifies the .get() fallback in verify_search_results is
defensive only (run_verifier always populates the entry)
- Eviction task-group guard narrowed from except Exception to
except RuntimeError (the only documented failure mode of
TaskGroup.start_soon on a closed group)
- Indexer logs a warning when a deck_card task is missing
board_id/stack_id, surfacing data-quality issues at index time
rather than at verification time
- New unit test covers the news verifier's non-numeric-id fail-open
path (one bad doc_id keeps the entire batch)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tightens verifier consistency, closes test gaps, hardens the fire-and-forget
eviction snapshot, and routes the new concurrency knob through Settings.
- Pre-flight ``int()`` guard in ``_verify_notes`` mirrors ``_verify_deck_cards``,
so a non-numeric note id produces a type-specific log line instead of
falling through to the generic "unexpected error" branch.
- Adds explicit 403 tests for the file and news verifiers (symmetry with the
existing notes/deck 403 tests) plus a ``non_numeric_id_keeps`` test.
- ``AppContext`` and ``OAuthAppContext`` no longer snapshot
``_vector_sync_state.eviction_task_group`` at lifespan-yield time. Both
expose it as a ``@property`` that reads the singleton dynamically, removing
the order-sensitive race where a future startup-ordering change could
silently degrade fire-and-forget eviction to inline forever.
- Adds ``verification_concurrency`` (env var ``VERIFICATION_CONCURRENCY``,
default 20) to ``Settings`` with a dynaconf validator; ``verify_search_results``
resolves the cap lazily from settings when the caller doesn't override it.
- Enriches the news verifier TODO to call out that ``batch_size=-1`` is
intentional — a numeric ceiling would silently break correctness because
any item beyond the cap would be missing from ``present_ids`` and dropped.
- Updates ``Optional[TaskGroup]`` to ``TaskGroup | None`` per project style.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Guard eviction_task_group.start_soon against shutdown race so a
RuntimeError on a closed group never surfaces as a search error.
- Correct ADR-019 news_item row: there is no per-item REST endpoint;
verification batches via get_items(batch_size=-1) and intersects.
- Modernize models/semantic.py typing to PEP 604 / lowercase generics
per CLAUDE.md.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- _verify_deck_cards: hoist int(board_id|stack_id|doc_id) out of the generic
except Exception into an explicit try/except (TypeError, ValueError) before
the network call, mirroring _verify_news_items. Malformed payloads now log
a specific warning instead of "unexpected error".
- _verify_news_items: add TODO(perf) above the get_items(batch_size=-1) call
to mark the known fetch-all cost as a future profiling target.
- SemanticSearchResult.id: revert from int|str back to int. The internal
SearchResult.id stays int|str for forward-compat; the MCP response model
narrows at the boundary. server/semantic.py casts r.id to int when
constructing the response so future string-id types fail loudly here
instead of silently widening the public API.
- nc_semantic_search: replace the terse "extra for access filtering" comment
with an ADR-019 NOTE block explaining the 2x over-fetch trade-off and the
ghost-density under-delivery case (self-heals via lazy eviction).
- tests/integration/test_verify_on_read.py: extend the module docstring to
call out that only the note verifier is exercised against real Nextcloud,
while file/deck_card/news_item are unit-only — documenting the suite split
for future contributors.
- ADR-019: rewrite "Module shape", "Verifier registry", example verifier,
and "Deduplication" sections to match the shipped BatchVerifier interface
(was per-id Verifier in the original draft). Add a "Why batch?" paragraph
explaining the design choice. Update implementation checklist — every
item is now [x] with corrected verifier names (plural) and the eviction
module path (vector/eviction.py).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements fire-and-forget eviction (ADR-019 §"Lazy eviction"): the
search response no longer waits on Qdrant deletes, instead spawning
evict() on a long-lived lifespan-owned task group. Falls back to inline
eviction in modes without vector sync and in unit tests.
Also: harden _verify_news_items against non-numeric ids (fail open
instead of crashing the verifier); document the get_file_info None-on-404
contract; add INDEXED_DOC_TYPES single source of truth in vector/scanner.py
referenced by the CI-guard test; write a Verify-on-Read Latency Budget
section in docs/configuration.md covering the unbounded news.get_items
fetch. Closes the two remaining ADR-019 implementation checklist items.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Cap all_results to limit*2 after sort in the per-doc_types branch of
nc_semantic_search to bound over-verification (was unbounded N-types).
- Switch BatchVerifier from (client, doc_ids, user_id) to (client, results,
semaphore). Verifiers now read file paths and deck board/stack ids from
SearchResult.metadata instead of doing fresh Qdrant scrolls — eliminates
one duplicate round-trip per file/deck-card verification.
- Bound per-id verification concurrency with a shared anyio.Semaphore
(default 20, matching server/semantic.py context-expansion convention).
Prevents httpx pool exhaustion / rate limiting on large search pages.
- Propagate stack_id from Qdrant payload to SearchResult.metadata in both
bm25_hybrid.py and semantic.py (board_id was already propagated).
- Drop now-unused _resolve_file_path / _resolve_deck_metadata helpers.
- Drop redundant int(d) in requested predicate from _verify_news_items.
- Rewrite eviction comment to be honest about inline (not background)
execution and the resulting latency coupling.
- ADR-019 status: Proposed -> Accepted.
- Add news property to NextcloudClientProtocol.
- Widen SearchResult.id and SemanticSearchResult.id to int | str to match
BatchVerifier signature and document support for future string-id types.
- Flip openWorldHint to True on nc_semantic_search_answer (it calls into
Nextcloud via nc_semantic_search).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The vector index lags Nextcloud (5-min webhook cron + scanner interval),
producing ghost records for deleted/unshared documents until the next
reconciliation. Verify each unique document against Nextcloud at query
time, drop inaccessible results, and lazily evict the corresponding
Qdrant points.
Per-doc_type batch verifiers: notes/files/deck cards run concurrently
per id; news items use a single fetch + intersect to avoid the per-item
fetch-all amplification. Transient errors fail open (keep result, log
warning) — only definitive 4xx drops. Multiple chunks of the same doc
collapse to one verification call.
Wired into nc_semantic_search before the limit trim and before context
expansion. nc_semantic_search_answer's per-note re-fetch retained as a
sub-second race guard since verification now happens upstream.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit addresses critical performance issues with vector visualization
search (reducing time from 40s to ~2s) and improves result visualization
through better visual encoding.
## Performance Fixes
### 1. Fix blocking sleep in retry decorator (base.py:51)
- Changed `time.sleep(5)` to `await anyio.sleep(5)` in @retry_on_429
- Prevents entire event loop from freezing during rate limit retries
- Impact: Reduced search time from 22s to 16s initially
### 2. Add concurrency limiting for verification (verification.py:77-93)
- Added `anyio.Semaphore(20)` to limit concurrent HTTP requests
- Prevents connection pool exhaustion (RequestError) from 90+ simultaneous requests
- Fixes false filtering (was filtering 77/90 results incorrectly)
- Note: Semaphore still in code but verification removed from viz endpoint
### 3. Remove unnecessary verification from viz endpoint (viz_routes.py:483-486)
- Visualization only needs Qdrant metadata (title, excerpt), not full content
- Verification only required for sampling (LLM needs full note content)
- Impact: Reduced search time from 43.7s to ~2s (final fix)
### 4. Restore streaming scanner pattern (scanner.py)
- Process notes one-at-a-time using async generator
- Avoids loading all notes into memory
## Visualization Improvements
### 5. Result-relative score normalization (viz_routes.py:489-504)
- Normalize scores within result set: best=1.0, worst=0.0
- Removes arbitrary RRF normalization (theoretical max didn't make sense)
- Makes visual encoding meaningful regardless of algorithm scores
### 6. Power scaling for marker sizes (userinfo_routes.py:743)
- Changed from linear `8 + (score * 12)` to power `6 + (score² * 14)`
- Creates dramatic visual contrast: 0.0→6px, 0.5→9.5px, 1.0→20px
- Combined with opacity (0.2-1.0) for clear visual hierarchy
### 7. Multi-channel visual encoding (userinfo_routes.py:740-745)
- Size: Exponentially scaled with score²
- Opacity: Linear 0.2-1.0 (keeps all points visible)
- Color: Viridis gradient (blue→yellow)
- Effect: Top results are large/bright/opaque, context results small/dim/transparent
## Result
- Search time: 40s → ~2s (20x faster)
- Visual contrast: Subtle → dramatic (clear result hierarchy)
- No arbitrary cutoffs: All results visible, best naturally highlighted
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Move access verification from individual search algorithms to final output
stage, eliminating redundant API calls and improving performance.
## Changes
**New:**
- `search/verification.py`: Centralized verification using anyio task groups
- Deduplicates results by (doc_id, doc_type) before verification
- Verifies all unique documents in parallel using structured concurrency
- Filters out inaccessible documents in single pass
**Modified Search Algorithms:**
- `search/semantic.py`: Removed _deduplicate_and_verify() and _verify_document_access()
- `search/keyword.py`: Removed _verify_access() and parallel verification
- `search/fuzzy.py`: Removed _verify_access() and parallel verification
- `search/hybrid.py`: Removed nextcloud_client parameter passing
All algorithms now return unverified results from Qdrant payload.
**Modified Output Stages:**
- `server/semantic.py`: Added verify_search_results() call after search
- `auth/viz_routes.py`: Added verify_search_results() call after search
Both endpoints now verify access once at final stage with deduplication.
## Performance Impact
**Before:**
- Hybrid mode (limit=10): 30 API calls (10 per algorithm × 3 algorithms)
- Single algorithm: 10-20 API calls (with verification buffer)
**After:**
- Hybrid mode (limit=10): 10 API calls (deduplicated verification)
- Single algorithm: 10 API calls (deduplicated verification)
**Performance Gain:** 3x reduction in API calls for hybrid search
## Architecture Benefits
- **Separation of concerns**: Algorithms handle scoring, output stage handles security
- **Deduplication**: Each document verified exactly once
- **Parallel execution**: All verifications run concurrently via anyio task groups
- **Consistency**: Same verification logic across MCP tools and viz endpoints
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
