embeddings_queries now records the embedding request's token count (the unit
upstream providers bill on) instead of an operation count, and fires on the
indexing path too. Previously only semantic search recorded it (value=1), so a
re-indexing run produced no embeddings_queries events at all — only pages_chunks.
- Provider layer: additive embed_with_usage / embed_batch_with_usage surface the
per-request token count (Mistral/OpenAI usage.total_tokens, Bedrock Titan
inputTextTokenCount, Ollama prompt_eval_count); a char-based estimate is the
fallback (Simple, and any provider/response without a token field). Gateway and
EmbeddingService forward through. The count travels as a return value / a
per-request SearchAlgorithm attribute — never on the singleton — so concurrent
indexing + search can't mis-attribute bills.
- Indexing (vector/processor.py): records embeddings_queries (value=batch tokens)
alongside the existing pages_chunks event.
- Search (server/semantic.py): value is now the query embedding's token count,
relayed from BM25HybridSearchAlgorithm via query_token_count.
The astrolabe_embeddings_queries Stripe meter (sum aggregation) now sums tokens
with no CP/Terraform change. The meter "queries"->tokens naming/unit
clarification (homelab-terraform #254) + CP rollup/portal copy is a follow-up.
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Simplify the OCS status guard to `if status and status != "ok"` — falsy
(missing/None/"") is tolerated more naturally than the explicit tuple.
- Add `test_value_error_from_ocs_failure_returns_none`, covering the
OCS-failure ValueError flowing through `_get_enabled_apps_or_none` to the
scan-all fallback.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Correct the misleading `test_malformed_envelope_returns_empty_set`
docstring: an empty-set return does NOT trigger the
`_get_enabled_apps_or_none` scan-all fallback (which fires only on
exceptions); optional apps are gated off for that cycle, Files unaffected.
- Check OCS `meta.status` in `get_enabled_apps`: a 200 carrying
`status != "ok"` now raises, so a 200-with-failure envelope routes through
the scanner's scan-all fallback instead of silently gating every app off.
Add a test for the failure-status raise.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Extract `_app_enabled` to a module-level helper so the gate predicate is
unit-tested directly instead of via an inline copy that could drift.
- Move `import logging` to module scope in test_scanner_app_gating.py.
- Harden `get_enabled_apps` OCS-envelope parsing (`X or {}` / `or []`) so a
present-but-null `ocs`/`data` coerces to empty instead of raising on
`None.get`; add parametrized malformed-envelope tests.
- Use https:// in the test request URL (SonarCloud S5332 hotspot).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The vector-sync scanner polled every indexed app (Notes, Files, News,
Deck) for every provisioned user on each scan cycle. When a user lacks
an app, its REST API returns 404; these were caught (indexing
continued) but flooded tenant logs with repeated 404s, scaling with
users x disabled-apps x scan-frequency and masking real failures.
Add NextcloudClient.get_enabled_apps(), which reads the per-user
/ocs/v2.php/core/navigation/apps endpoint (respects group
restrictions). Chosen over /cloud/capabilities because the News app
advertises no capability and never appears there.
scan_user_documents now resolves the enabled-app set once per cycle and
skips the Notes/News/Deck scans for apps the user lacks. Files stays
unconditional (core Tags API, not a 404 source). Detection failures
fall back to scanning every app (prior behaviour), so a transient
nav-endpoint blip never silently halts indexing; the per-app 404 guards
remain as the safety net.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Assert the open card stays visible under status="open" in the
deck_get_stack integration test (completes the partition check).
- Move the _append_archived_cards docstring closing quotes to their own line.
deck_get_stack's status="archived" + include_cards=False path is left as-is:
a single get_stack call is the cheapest way to obtain the stack metadata
there — routing it through the archived fast-path would fetch every archived
stack on the board just to strip the cards, which is heavier, not lighter.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- deck_get_stack: fetch active + archived concurrently for status="all", and
for status="archived" source the stack from /stacks/archived in a single
call (skip the active fetch whose open cards are filtered out anyway),
matching deck_get_cards' pattern.
- Type the `client` param of _archived_cards_by_stack as NextcloudClient.
- Extend the stacks/overview integration test to assert status="archived"
(only the archived card) in addition to status="all".
- Document the third_party/astrolabe submodule mount policy in CLAUDE.md:
unmounted by default (CI installs the published app-store version); mount
only for tightly-coupled feature work needing CI integration, then revert.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The active Deck listing endpoints (StackService::findAll /
CardMapper::findAllForStacks and StackService::find / CardMapper::findAll)
filter out archived cards at the SQL level — only the /stacks/archived
endpoint returns them. The client-side status="all"/"archived" filters in
deck_get_cards, deck_get_stacks, deck_get_stack and deck_get_board_overview
therefore operated on a list the server had already stripped of archived
cards, so they could never surface one. deck_get_card (by ID) bypasses the
filter, which is why it appeared to work. Fixes#842.
When status is "all" or "archived", also fetch /stacks/archived
(client.deck.get_archived_stacks) and merge those cards back in per stack —
concurrently with the active fetch where applicable. status="open"/"done"
are unchanged and cost no extra call.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- semantic.py: normalize both None and [] doc_types to null in the
metadata so a future `metadata->'doc_types' IS NULL` query counts the
all-types case consistently.
- test: use a fixed past date in test_occurred_at_roundtrip instead of a
future literal (deterministic, no "why this date" confusion).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Non-blocking follow-ups from the merge-ready review:
- semantic.py: bound the doc_types copied into embeddings_queries metadata
to _USAGE_METADATA_MAX_DOC_TYPES (16). doc_types is caller-supplied with
no max_length on the tool signature; capping the stored copy keeps one
JSONB row from ballooning (not a billing/injection risk — CP ignores
metadata, binds are parameterized).
- migration: note that `metric` is intentionally unconstrained Text and
that adding a third metric requires keeping the CP-side catalog in sync,
else the rollup silently ignores the new rows.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- hooks: document why user_id in metadata is safe — it stays tenant-local
(the CP rollup aggregates GROUP BY (day, metric) into usage_daily, which
has no metadata column, so it never reaches Stripe) and is retained to
keep Deck #67's future per-user attribution derivable from the app DB.
- migration: instantiate the SQLite-side column types (sa.Text() etc.) for
visual parity with the instantiated Postgres types.
- tests: assert the WARNING contract in the unserializable-metadata test
too; add an autouse fixture that resets UsageEventStore._shared_instance
so a stray shared() call can't leak across tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- store: guard UsageEventStore.shared() with a class-level anyio.Lock so
two concurrent cold-start callers don't both build (and one silently
overwrite) the cached instance — mirrors get_shared_storage(). Document
that tests should construct the store directly to avoid singleton leak.
- migration: rename 20260610 -> 20260607 and fix Create Date to today so
`alembic history` isn't future-dated (revision id 007 / down_revision
006 unchanged; single head verified).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- remove accidentally-committed .claude/scheduled_tasks.lock (Claude Code
runtime artifact swept in by `git add -A`) and gitignore it; the rest
of .claude/ stays tracked.
- store: cache UsageEventStore.shared() as a process-wide instance so the
hot search path doesn't allocate a fresh wrapper per metered query (the
wrapper is stateless beyond its storage handle).
- hooks: pass enabled=True directly (the outer guard already confirmed
the flag) instead of re-reading settings.usage_metering_enabled.
- migration: document the no-TTL retention design (control-plane rollup
owns the lifecycle; the data plane only appends).
- tests: assert the best-effort error path logs at WARNING (observability
contract).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- store: add optional `enabled` param to record_usage_event so hot-path
callers (nc_semantic_search) pass the already-resolved flag instead of
forcing a second uncached Settings build (ADR-024); falls back to
get_settings() when None so the store stays self-gating for standalone
use.
- hooks: thread enabled= through both call sites; bump the outer
shared()/construction failure log from debug → warning so "metering
enabled but no billing data" is visible at the default INFO level.
- migration: instantiate postgresql.JSONB() to match the sibling
TIMESTAMP(timezone=True) column.
- tests: fix the misleading "asyncpg returns JSONB as a JSON string"
comment; add occurred_at dialect round-trip test and an enabled-param
short-circuit test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deck #67 data-plane slice: tenant Pods record billable operations
(embedding queries, pages/chunks embedded) into an app-DB usage_events
table that the control plane later pulls read-only into the billing
ledger and syncs to Stripe Meter Events.
- migration 007: usage_events table (Postgres TIMESTAMPTZ/JSONB/UUID
with portable SQLite fallbacks), indexed (occurred_at, metric) for the
CP rollup's per-day range scan + GROUP BY metric.
- UsageEventStore: best-effort, flag-gated writer reusing the shared
RefreshTokenStorage engine; ON CONFLICT (event_id) DO NOTHING for
idempotent retries; dialect-branched occurred_at bind. All work
(incl. metadata JSON encode) is swallowed so a metering failure never
surfaces to the user op.
- USAGE_METERING_ENABLED flag (default off) wired through Settings +
env map; off-path touches no storage, so OSS self-hosters get an empty
table and zero write overhead.
- two recording hooks: embeddings_queries (per nc_semantic_search, which
nc_semantic_search_answer reuses) and pages_chunks (after dense
embedding succeeds, covering both in-process and procrastinate paths).
- storage.acquire()/.dialect public seams so the sibling store doesn't
reach into the underscored internal.
- tests parametrized over SQLite + Postgres: flag-off no-op, roundtrip,
ON CONFLICT dedup, JSON/NULL metadata, and the best-effort swallow of
both DB errors and unserializable metadata.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 5 on PR #868: add
test_all_blank_pages_returns_empty_list documenting that PageAwareChunker
returns [] when every page is blank — and asserting parity with
DocumentChunker, which already returns [] for whitespace-only non-empty
content. The empty-chunk-list case is therefore pre-existing pipeline
behavior, not new to this PR.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 4 on PR #868: tighten the assign_page_numbers
guard from `page_boundaries is not None` to a truthy check, so a PDF with an
empty boundary list no longer enters the trace span and fires the alarming
"NO page numbers assigned" warning for a harmless no-op.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 3 on PR #868: add module-level
`pytestmark = pytest.mark.unit` so TestPageAwareChunker and
TestDocumentChunkerPositions are collected under `-m unit`, matching
test_processor_routing.py.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 2 on PR #868:
- Extract the use_page_aware branching into a pure `should_use_page_aware`
helper and cover the (doc_type, page_boundaries, page_aware_setting) matrix
in tests/unit/test_processor_routing.py (file+boundaries+enabled, empty
list, None, non-file doc types, disabled setting).
- Clarify the PageAwareChunker.chunk_text no-boundaries comment: the processor
pre-filters via should_use_page_aware, so that branch is a direct-call safety
net, not a production indexing path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 1 on PR #868:
- use_page_aware now gates on `bool(page_boundaries)` instead of
`is not None`, so a PDF that yields an empty boundary list takes the
char-based path explicitly (assign_page_numbers no-ops on []) rather than
the page-aware chunker's no-boundaries fallback. Same result, clearer intent.
- add test_oversized_page_with_leading_whitespace_offsets, exercising the
start+start_index offset path for an oversized page whose sub-chunks have
leading whitespace.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DOCUMENT_CHUNK_SIZE/OVERLAP were documented as "words" with a 512/50
default; the implementation measures characters and defaults to 2048/200
(config.py, DocumentChunker). Update docs/configuration.md (config block,
tuning guidance, examples, env-var table) and env.sample accordingly, and
cross-reference DOCUMENT_CHUNK_PAGE_AWARE for the PDF path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add PageAwareChunker, which splits paginated documents (PDFs) on page
boundaries first and only character-splits pages larger than chunk_size.
No chunk spans a page boundary, so page_number is always exact and stored
excerpts never lead with a neighbouring page's text. When chunk_size is at
least the largest page, this yields exactly one chunk per page: a
predictable vector count (== page count), a flat per-page embedding cost,
and zero cross-page overlap duplication.
Gated by DOCUMENT_CHUNK_PAGE_AWARE (default true). When false, the legacy
char-based DocumentChunker + post-hoc assign_page_numbers path runs
unchanged. Only doc_type="file" with page_boundaries (PDFs) takes the
page-aware path; notes/deck/news are unaffected.
Measured on a 15-page record (query "leadership award louis", target =
top-half of page 15): char-based degraded the target to dense-rank 10 at
cs=2048 (OCR) and mislabeled its page; page-aware restored rank 1 across
every fusion/modality and chunk size, with correct page labels and clean
snippets.
BREAKING CHANGE: PDFs are re-chunked page-aware by default. Existing
deployments will re-index PDF content on the next vector sync (different
chunk counts and page_number labels). Set DOCUMENT_CHUNK_PAGE_AWARE=false
to retain the previous char-based behaviour.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Tagging an existing file/folder emits only OCP\SystemTag\MapperEvent — never a
Node*Event — so tagged PDFs were previously only picked up by the hourly
scanner. Subscribe to the tag event and reconcile membership so adding/removing
the `vector-index` tag (re)indexes in near-real time.
- webhook_presets: add OCP\SystemTag\MapperEvent to the files_sync preset
(NC 32+, where MapperEvent gained getWebhookSerializable(); harmless on older
servers — it just never fires).
- webhook_parser: parse MapperEvent (objectType=files) into a path-less file
"reconcile" task. The payload carries only a fileid + tagIds (no name/path),
so assign and unassign both collapse to a reconcile.
- processor._reconcile_tag_event: resolve the fileid against the user's current
vector-index PDFs (find_files_by_tag). Present -> index with the resolved
path/etag; absent -> flip to delete. Naturally handles "an unrelated tag
changed" and a tagged folder's own fileid (no-op; the scanner still expands
folders to descendants).
- Unit tests for the parser branch and the reconcile.
The matching admin-UI preset change ships separately in the astrolabe app repo.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #863 round 4: classify_from_text's docstring now states that the
image_heavy flag (and the image-coverage trigger) are only set when
image_coverage is supplied, so the flag reads zero for tenants with
DOCUMENT_OCR_DETECT_SCANNED=false -- self-documenting the metric semantics.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #863 round 3:
- classify_from_text emits the "scanned" flag (was "no_text_layer") for the
empty-text-layer case -- same name + meaning as classify_pdf, so
astrolabe_document_classifier_flag_total isn't split across two labels for the
same concept (and matches the metric's documented vocab).
- classify_from_text logs at DEBUG when image_coverage length != the expected
min(pages, MAX_SAMPLED_PAGES), so a 1:1-alignment contract break (extractor
reorders/skips pages) surfaces instead of silently misattributing coverage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #863 round 2:
- classify_pdf now flags a page needs_ocr on the SAME three signals as
classify_from_text (image scan OR low text-quality OR near-empty), not image
coverage alone. Previously a word-merged digital doc with no images routed
"fast" via classify_pdf but "ocr" via the pipeline -- so an operator
reproducing routing offline got a different answer. They now match.
- Add a test that when image_coverage is shorter than the page boundaries (the
MAX_SAMPLED_PAGES cap on large scans), the leading page uses the scan signal
and later pages fall back to text-quality.
Left as-is: overlong_score (>20) partially overlaps merge_score (>12) -- the
double-penalty on very-long tokens is intentional, not a bug (per review).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #863 review:
- MIN_TEXT_QUALITY 0.45 -> 0.5 so the module/diagnostic default matches the
DOCUMENT_OCR_MIN_TEXT_QUALITY setting (registry always passes the setting; this
keeps classify_pdf and the test/default path on the production threshold).
- image_coverage_per_page is bounded to MAX_SAMPLED_PAGES (the image pass is the
costly part, so a 200-page scan isn't fully rasterised on the hot path); pages
beyond the cap fall back to the text-quality signal, and page_fraction still
gates over every page.
- Extracted _page_image_coverage(page) helper, shared by classify_pdf and
image_coverage_per_page (DRY + keeps the tiling-double-count note in one place).
- Scan-detection failure logs at WARNING (not DEBUG) so a systematic failure on
an OCR-enabled tenant is visible at LOG_LEVEL=INFO.
- Add the missing DOCUMENT_OCR_MIN_PAGE_CHARS range-validator test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The hot-path classifier escalated to OCR purely on character count, so a
scanned/handwritten PDF with a low-quality embedded text layer (>16 chars/page
but garbled) routed `fast` and indexed the junk -- e.g. Student 147.pdf's
"Little Acoms Primary"/"0110912020", which pollutes the vector and demotes the
doc in search (Deck #207).
- classifier: recalibrate `_text_quality` with a long-token-fraction term that
detects word-merging (dropped inter-word spaces) -- the dominant junk-layer
failure the old whitespace/overlong(>20) terms missed. Measured: the Student
147 scan ~0.42 (60% pages junk) vs >=0.94 for clean digital docs.
- classify_from_text now routes on quality + scan: a page is OCR-worthy if
near-empty OR low text-quality OR (when OCR + scan detection are enabled) it's
mostly a raster image. New `image_coverage_per_page` re-opens the PDF for the
scan signal, so that cost is paid only by OCR-opted-in tenants. Thresholds are
passed in from per-tenant settings (keyword-only).
- config: 4 per-tenant settings -- DOCUMENT_OCR_MIN_TEXT_QUALITY (0.5),
DOCUMENT_OCR_PAGE_FRACTION (0.5), DOCUMENT_OCR_MIN_PAGE_CHARS (16),
DOCUMENT_OCR_DETECT_SCANNED (true) -- with range validators.
- metrics: new astrolabe_document_ocr_page_fraction histogram (the value the
page-fraction threshold acts on) alongside document_text_quality, so operators
can tune the OCR escalation per tenant (quality vs cost).
Escalation gate, OCR backends, and off-by-default behavior unchanged (#858).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The high ocr_frac is driven by each segment being shorter than MIN_PAGE_CHARS
(needs_ocr), not by text quality; quality drives bad_text_layer separately.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to the tiered document processor (#858), landing the round-4 review
nits the reviewer approved without:
- pypdfium2_fast: free the page handle in an outer finally so a corrupt page
that makes get_textpage() raise can't orphan it.
- test: classify_from_text junk-text-layer path (non-zero chars, low quality,
high ocr_frac) flags bad_text_layer -- the hot-path coverage gap.
- test: build_ocr_backend raises ValueError when the gateway M2M client_id is
set without its token_url/secret.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #858 review round 3:
- document_tier1_engine / document_ocr_provider now validate + normalize via
Settings.__post_init__ _enum_fields (the repo's canonical opt-in-enum pattern;
case-insensitive) instead of dynaconf Validators. A typo now raises ValueError
at load and "Gateway" normalizes to "gateway".
- classify_from_text gates no_text_layer/bad_text_layer on ocr_frac >=
OCR_PAGE_FRACTION, matching classify_pdf -- a "fast"-routed doc with a few junk
pages no longer emits a misleading flag (keeps the shadow vs hot-path
classification metrics consistent).
- build_ocr_backend warns when an EXPLICIT provider is misconfigured
(gateway without EMBEDDING_GATEWAY_URL, mistral without MISTRAL_API_KEY)
instead of silently returning None.
- Pypdfium2FastProcessor.health_check probes the import; documented why
OcrProcessor.health_check is unconditionally True (lazy per-tenant backends).
- Removed the leftover per-boundary / per-chunk debug logging loops.
Tests: enum normalization + rejection for the two new settings.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #858 review round 2:
- OcrProcessor backend resolution is now guarded by an anyio.Lock (lazy-init,
double-checked) so a burst of concurrent first-OCR calls resolves the backend
once instead of each fetching its own gateway M2M token.
- The document_tier1_engine=pymupdf rollback now logs a warning when it falls
back to the fast processor (no 'structured' registered) instead of silently
using the very engine the operator opted out of.
- classify_from_text defaults ocr_frac to 0.0 (not 1.0) for a zero-page PDF, so
the recorded classification metric is "fast" (no OCR evidence) rather than a
misleading "ocr"; the no_text_layer/bad_text_layer flags are gated on having
sampled at least one page.
New tests: zero-page classify routes fast, rollback-fallback warning.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #858 review:
- 🔴 OcrProcessor now resolves its backend once and reuses it. Rebuilding per
call created a fresh GatewayTokenProvider each time -- discarding its M2M-token
cache, so every OCR'd document fetched a new token -- and a new Mistral client.
- 🔴 build_ocr_backend uses explicit ValueError (not assert, which is stripped
under `python -O`) for the gateway M2M triple.
- PIPELINE_TIER in the Qdrant payload now reflects the tier that actually
produced the doc: the registry stamps result.metadata["pipeline_tier"] and the
processor reads it (was hardcoded "fast", wrong for OCR/structured).
- Escalation now requires classification.page_count > 0, so a zero-page
(empty/corrupt) PDF isn't pointlessly sent to OCR; documented that a fast
FAILURE (encrypted/unopenable) is a hard failure and is not OCR-escalated.
- Documented the OCR page_boundaries separator-attribution choice.
- Downgraded the per-document page-boundary / page-assignment INFO logs to debug.
New tests: zero-page no-escalation, pipeline_tier stamping.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OCR is an enhancement, not a gate. Previously, escalating a scanned doc to the
OCR tier returned the OCR result unconditionally -- so with DOCUMENT_OCR_ENABLED
=true but no backend configured (no gateway URL / no MISTRAL_API_KEY) the OCR
processor returned success=False and the whole document was marked failed and
skipped: strictly worse than leaving OCR off (where it would at least index the
tier-1 text).
Now the registry keeps the tier-1 fast result when the OCR escalation doesn't
succeed (no backend, API down, empty output), logging a warning. A
misconfiguration degrades gracefully instead of dropping scanned docs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the OCR escalation target the tiered registry already routes to. Scanned /
no-text-layer PDFs (the tier-0 "ocr" verdict) escalate here when
document_ocr_enabled (default off).
Two interchangeable backends, selected by document_ocr_provider
(auto | gateway | mistral | none):
- gateway: POST to the Astrolabe Cloud model gateway's /v1/ocr -- the same
M2M-authenticated gateway as embeddings, so NO provider keys live in the pod
(the platform default; reuses EMBEDDING_GATEWAY_URL + the M2M creds).
- mistral: call the Mistral OCR API directly from the pod (MISTRAL_API_KEY), for
self-hosters / deployments without the gateway.
"auto" prefers the gateway, then direct Mistral.
Both return per-page markdown joined into text + exact page_boundaries (the
pdf_highlighter contract; bbox re-derived from the PDF bytes as for other tiers).
Validated end-to-end via direct Mistral on the scanned Student 147.pdf:
success, 15 pages, 22k chars, offsets exact, ~4s.
Settings: document_ocr_provider (enum-validated), document_ocr_model
("mistral/mistral-ocr-latest" -- gateway routes on the prefix, the direct mistral
backend strips it). OcrProcessor registered at lowest priority so it is never the
non-tiered default.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces single-engine pymupdf4llm extraction with a tiered pipeline (Deck #205,
follows the tier-0 classifier #855). pypdfium2 becomes the default and only
hot-path PDF extractor; pymupdf4llm is deprecated to a rollback toggle.
Why: pymupdf4llm's O(n^2) find_tables drove the OOM (#852) and the form-PDF
parse timeouts (#856), carries AGPL/commercial licensing liability, and -- per
the benchmarks -- recovers near-zero usable tables on the real corpus. pypdfium2
(Apache/BSD) extracts the same text far faster (Student 1a.pdf: 120s timeout ->
0.2s) with no table-detection bomb.
- document_processors/pypdfium2_fast.py: tier-1 "fast" processor emitting text +
exact page_boundaries (the pdf_highlighter contract). pymupdf processor is now
tier "structured" (the rollback engine), registered but not default.
- registry: tiered routing in ProcessorRegistry. tier-1 fast extracts, then
classification is DERIVED from that text (classifier.classify_from_text -- no
PDF re-open), records the classification metrics, and escalates scanned /
no-text-layer docs to the "ocr" tier when document_ocr_enabled (default off;
no provider yet, so fast is terminal). Wires record_document_escalation + the
real "escalated" span attribute (was hardcoded False).
- Removes the separate _shadow_classify pass from vector/processor.py -- it
re-opened every PDF and re-extracted text (~0.5-1.3s/doc of pure duplicated
CPU that lowered throughput); classification now rides the tier-1 extraction.
- Settings: document_tier1_engine ("pypdfium2" default | "pymupdf" rollback,
enum-validated), document_ocr_enabled (default false).
Tests: pypdfium2 extractor, registry tiering (fast routing, rollback, classify
recording, OCR escalation on/off), classify_from_text. Full unit suite green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 review follow-ups (PR #857), both documentation-only:
- sharing_state.py: reconcile_document_path docstring no longer claims it
returns False when no real points exist — it returns True and the set_payload
is a Qdrant-side no-op (callers discard the return value).
- scanner.py: reword the rename-reconcile comment to state the precise reason
(modified_at stable so not re-queued; path may be stale from a rename) rather
than the loose "dedup miss / etag changed" phrasing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review follow-ups (PR #857):
- scanner.py: skip the rename-reconcile when the existing metadata point is a
placeholder. reconcile_document_path only touches real chunks, so a not-yet-
indexed file would just incur a 0-point set_payload; the real index writes the
current path anyway.
- test_sharing_state.py: add a dedup-hit case where the file was renamed AND the
user is new to the ACL, asserting both set_payload writes fire (file_path/title
and acl_principals).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The vector-sync pipeline derived an indexed file's display title from the
document's embedded metadata (e.g. a PDF's /Title), falling back to the
filename only when absent. That embedded title frequently disagrees with how
the user named the file in Nextcloud and is confusing in the astrolabe
vector-viz UI (a passive consumer of the `title` payload field).
For files, always derive the title from the Nextcloud filename via a shared
`file_title_from_path` helper. Notes/deck/news keep their metadata titles.
A rename/move in Nextcloud keeps the fileid (doc_id) and content (etag/mtime)
but changes the path, so both the dedup claim and the scanner freshness gate
skip re-embedding and the stored file_path/title go stale. Add
`reconcile_document_path`: a metadata-only set_payload that refreshes
file_path + title on the existing real chunks without re-fetch/re-embed.
Wire it into both skip paths:
- dedup hit (etag unchanged on rename) via claim_existing_index(current_path=...)
- scanner incremental skip (etag changed, mtime stable)
Both reuse already-fetched payloads, so steady-state scans add no extra
round-trip (reconcile is a no-op when the path is unchanged).
Refs: Deck #204
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SonarQube flagged three float equality checks in the classifier tests
(python:S1244, "do not perform equality checks with floating point values"):
the _text_quality empty case and the ocr_page_fraction 0.0/1.0 assertions now
use pytest.approx.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #855 round 3 (non-blocking test completeness):
- Add a test that a mostly-digital doc with one full-page image carries the
image_heavy flag yet still routes fast (ocr_frac < OCR_PAGE_FRACTION) -- the
flag-vs-routing asymmetry operators read in the metrics, now guarded against
silent regression.
- test_full_page_image_routes_ocr also asserts the scanned flag (no text layer).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #855 round 2:
- 🔴 _shadow_classify swallowed all exceptions at DEBUG, so a systematic
failure (pymupdf bug, memory pressure) is invisible at LOG_LEVEL=INFO and
trips SonarQube S2221/S5754. Log at WARNING instead (still best-effort --
indexing is unaffected).
- classifier: use `with pymupdf.open(...) as doc` instead of manual try/finally.
- tests: release the Pixmap's native memory (del pix) in the image fixtures.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #855 review (all non-blocking):
- classifier: _sample_indices now always includes the first AND last page (the
old evenly-spaced sample missed the tail, e.g. last sampled index 95 on a
100-page doc -- a scanned tail could be missed).
- classifier + metrics: document that flags are diagnostic and fire
independently of routing (image_heavy on ANY page vs the ocr route needing a
page FRACTION), so flag{image_heavy} is expected to exceed classified{ocr}.
- classifier: clarify the text-quality whitespace comment (caps at 12%) and note
the image double-count approximation (min() caps coverage).
- tests: add the scanned (no text layer) and bad_text_layer (junk text over an
image) flag paths, and a test pinning first/last-page sampling.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
First step of the tiered document-processor effort (Deck #203): a cheap, local
pre-pass that recommends which extraction tier a PDF should start in, emitting
metrics WITHOUT changing routing yet -- so we gather per-tenant doc-mix data
before turning escalation on.
document_processors/classifier.py: classify_pdf(content) -> DocClassification.
Page-sampled (bounded on large docs), <~1s. Cheap signals only -- text-layer
chars, a text-quality score (catches the "Student 147" failure where a text
layer exists but is mashed/space-less junk), and image coverage. A page that is
mostly a raster image routes to OCR: its content (handwriting, stamps) isn't in
any text layer. Deliberately no get_drawings/graphics-density signal -- it's
slow on the exact pages it'd flag, the hotfix's graphics_limit already makes the
parse safe, and the (future) tier-1 quality gate catches lost tables.
Validated on the sample corpus: born-digital 2-col arxiv and a digital student
record -> fast (tier 1); a scanned+handwritten form -> ocr (tier 3).
Wiring (vector/processor.py): _shadow_classify runs the classifier on PDFs in a
worker thread, best-effort (never blocks/fails indexing), gated by the new
DOCUMENT_CLASSIFY_ENABLED setting. Metrics: astrolabe_document_classified_total
{recommended_tier}, astrolabe_document_classifier_flag_total{flag},
astrolabe_document_text_quality histogram.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The OOM hotfix (#852) set graphics_limit=5000, which caught the 955k-drawing
bomb but let a second pathology through: form/table PDFs (e.g. student records)
have ~1.5k grid-line vector drawings per page -- under 5000, so uncapped. With
those pages uncapped, pymupdf4llm's O(n^2) find_tables grinds ~17s/page, so a
7-page form hits the 120s timeout. All 6 current backfill parse failures in
tenant-blackbox-demo are this exact timeout (zero OOM, zero error).
Measured on a 7-page sample: graphics_limit=2000 -> 119s (timeout), 1000 -> 2.9s
-- with identical extracted text and ZERO recovered tables either way (the
expensive analysis produces nothing useful on these dense forms). Lowering the
default to 1000 makes them index in ~3s; the bomb file (955k >> 1000) stays
capped, and pages with genuine simple tables (<1000 drawings) still get table
detection.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Lowering VECTOR_SYNC_SCAN_INTERVAL to 5s (previous commit) fixed user
discovery on nc31 but exposed re-scan churn on the slower nc32 runner: each
scan re-queues the user's entire corpus, so a 5s cadence floods the single
processor worker faster than it drains (pending climbed to 20-30+ while
indexed stayed 0, status "syncing").
Discovery latency and re-scan churn are separate knobs. Keep
USER_POLL_INTERVAL short (5s) for prompt discovery — the per-user scanner
runs its initial scan immediately on start, so the corpus is queued once
right away — but restore a moderate SCAN_INTERVAL (30s) so re-scans don't
re-flood the queue. Indexing of the one-time initial scan completes well
inside the test's 90s budget; a note created just after that scan is still
picked up by the 30s re-scan.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
After restoring the background-sync UI ids, the two indexing-dependent
multi-user-basic tests (chunk_context_uses_app_password, plotly_with_basic_auth)
surfaced a second, previously-masked failure: they provision a user, create a
note, then wait ~90s for it to be indexed.
The mcp-multi-user-basic service ran with the production cadence — scan
interval 60s and the default user-poll interval 60s. A freshly-provisioned
user isn't even *discovered* by the background-sync user manager for up to
60s, leaving too little of the 90s budget for the scan + single-worker
indexing to finish (observed: pending docs still "syncing" at timeout, or the
scanner not yet started → "idle" with 0 indexed).
Match the single-user service's short cadence (5s) and add a matching 5s
user-poll interval so discovery + scan + index complete well within the test
budget. Test-only config; production deployments set their own intervals.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Astrolabe PHP→Vue settings refactor dropped three stable element ids
(#mcp-enable-background-button, #mcp-revoke-background-button,
#mcp-revoke-background-form) that the multi-user-basic integration suite
drives the background-sync enable/disable/revoke flows through. Their
absence timed out the 5s Playwright locators and failed four tests:
- test_astrolabe_multi_user_background_sync::test_multi_user_astrolabe_background_sync_enablement
- test_astrolabe_multi_user_background_sync::test_revoke_background_sync_access
- test_astrolabe_chunk_context::test_chunk_context_endpoint_uses_app_password
- test_astrolabe_plotly_visualization::test_astrolabe_plotly_visualization_with_basic_auth
(the latter two enable background sync via complete_astrolabe_authorization
before exercising the app-password / indexed-search paths).
Two-part fix:
1. Bump the astrolabe submodule to v0.20.1 (cbcoutinho/astrolabe#116),
which restores the three element ids on the refactored NcButtons.
2. Defense-in-depth in the test helpers: resolve the enable/revoke buttons
by their stable id first, falling back to the button's accessible name
so a future id rename degrades to a slower-but-working lookup instead of
a hard timeout. Avoids a combined `.or_()` locator, which would
strict-mode-violate (the id button also matches by text).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #852 round 3 (all 🟡, no blockers):
- config: DOCUMENT_PARSE_TIMEOUT_SECONDS is now float (default 120.0) so a
fractional value is honoured rather than silently stored in an int field;
matches anyio.move_on_after's float seconds.
- _isolation: comment that a clean rlimit MemoryError leaves the worker alive
in anyio's pool (vs the SIGKILL/BrokenWorkerProcess path that respawns) --
acceptable since RLIMIT_AS caps virtual address space, not RSS.
- processor: note the `if indexed is False` is a deliberate identity check --
a successful index (incl. dedup hit) returns None and must not be mistaken
for a parse failure.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #852 round 2:
- config: DOCUMENT_PDF_GRAPHICS_LIMIT validator is now gte=1 (pymupdf4llm treats
0 as "no cap", which would re-expose the OOM); documented the zero semantics
and that the per-worker mem rlimit needs a pod restart to change.
- processor: annotate `_index_document -> bool | None` and document the contract
so the `if indexed is False` check is explicit/type-checkable.
- tests: add the RLIM_INFINITY-hard branch assertion for _apply_mem_limit
(soft==target, hard stays unbounded).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #852 review:
- pymupdf.py: the metadata `doc` was only closed on the PdfParseFailed and
success paths, so a failure in `_extract_metadata`/`mkdir`/`get_settings`
leaked it. `doc` is only needed for metadata + page_count (the heavy parse
works from `content` bytes in the worker), so open it, read metadata, and
close it immediately under try/finally; drop the two later doc.close() calls.
- processor.py: a permanent parse failure early-returned from `_index_document`,
after which `process_document` still recorded record_qdrant_operation("upsert",
"success") + record_vector_sync_processing(success) -- counting an OOM/timeout
bomb as astrolabe_documents_indexed_total{status="success"}. `_index_document`
now returns False on that path and the caller skips the success metrics (the
failure is already recorded via document_parse_failed_total + the registry's
document_parse_total{error}).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The document processor crash-looped on one pathological PDF: pymupdf4llm's
table/graphics detection over a page with ~1M vector path items ballooned past
the 2 GiB pod limit. The parse ran in a thread, so nothing could interrupt or
memory-bound it -- a single bad file OOM-killed the whole pod.
Run the parse in an isolated worker subprocess (anyio.to_process, cancellable)
with an RLIMIT_AS memory cap and a wall-clock timeout, so a pathological file
fails THAT document instead of the pod (new document_processors/_isolation.py).
Also pass graphics_limit (default 5000) to to_markdown -- validated to cut the
known trigger page from 112 s to 23 s with bounded memory.
On a permanent parse failure the processor returns success=False (instead of
raising, which would retry 3x); vector/processor.py marks the placeholder
"failed" and skips indexing, and the scanner stops re-queuing failed placeholders
until the file changes -- so a doomed file no longer churns.
New per-tenant (per-pod env) settings: DOCUMENT_PDF_GRAPHICS_LIMIT,
DOCUMENT_PARSE_TIMEOUT_SECONDS, DOCUMENT_PARSE_MEM_LIMIT_MB. New metric
astrolabe_document_parse_failed_total{reason=timeout|oom|error} surfaces hard
failures that previously killed the process before any except ran.
First PR of the tiered document-processor effort (Deck #199); tier 0/1/3
pipeline tracked separately.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Starlette lifespan started `vector_sync_metrics_task` with undefined
names `task_producer` and `receive_stream`. Those locals only exist inside
the `_wire_vector_sync_state` helper; in the lifespan the transport is bound
as `ingest_transport`. The undefined reference raised `NameError`, which
aborted the background-sync task group and crashed startup in every
deployment mode ("Application startup failed. Exiting.").
Introduced by fbe70ecd ("feat: backend-agnostic vector-sync gauges").
Pass `ingest_transport.producer` / `ingest_transport.receive_stream` at both
call sites (single-user app.py:1791, OAuth/login-flow app.py:2012).
Also fix 10 pre-existing `ty` possibly-missing-attribute diagnostics: the
deck indexing code in scanner.py, processor.py and search/context.py reads
full-DeckCard-only fields (description, type, owner, etag, lastModified) off
`stack.cards`, typed `list[DeckCard | DeckCardSummary]`. Freshly-fetched
stacks from `get_stacks()` always hold full DeckCards (the summary
projection only happens in the tool layer), so narrow with
`cast(list[DeckCard], ...)` — matching the existing pattern in
server/deck.py.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
LocalTransport.aclose() (added in round 3) closes its owned stream ends; the ADR
still described aclose() as a no-op for the memory stream. Update the prose to
match the shipped behaviour. Doc-only.
Refs: Deck #196
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- _clear_vector_sync_state also nulls shutdown_event / scanner_wake_event on
shutdown, symmetric with the stream/producer fields (the next startup rebinds
them via _wire_vector_sync_state).
- Comment that the "DocumentTask" string subscript in LocalTransport is
intentional (TYPE_CHECKING-only class; anyio ignores the runtime type arg).
- Move app.py's annotation-only IngestTransport / TaskProducer imports under
TYPE_CHECKING (the module uses `from __future__ import annotations`), keeping
only build_transport at runtime.
Refs: Deck #196
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Clear the module-singleton ingest references (task_producer,
document_send_stream, document_receive_stream) on lifespan shutdown via a new
_clear_vector_sync_state() helper, mirroring the eviction_task_group cleanup.
Defense-in-depth so a late webhook (or a module-singleton integration test)
can't touch a producer/stream backed by an already-closed resource.
- Add IngestTransport.backend_name ("memory"/"postgres") and use it in both
lifespan log lines, removing the last settings.ingest_queue read from the
background-sync setup — the lifespan no longer inspects the backend at all.
- Cover backend_name in the build_transport adapter-selection tests.
Refs: Deck #196
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- LocalTransport.run_consumers increments active_consumer_count per worker
(instead of once after the loop) so the count is accurate if a later
tg.start() raises mid-pool.
- Add LocalTransport.aclose() to explicitly close its owned send/receive stream
ends (belt-and-suspenders against unclosed-resource warnings; anyio aclose is
idempotent, and by shutdown the scanner is already winding down). Reworded the
base IngestTransport.aclose() docstring to point at the overrides.
- Inline ingest_transport.producer at the scanner/user_manager call sites,
dropping the single-use task_producer alias in both lifespan paths.
- Annotate DistributedTransport._producer explicitly as ProcrastinateTaskProducer
so the drain() coupling is visible and ty catches drift.
- Add a unit test for LocalTransport.aclose() (closes the owned streams,
idempotent).
Refs: Deck #196
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extend the documents-vs-chunks split to the remaining status surfaces so all
three report consistently (Deck #195):
- nc_get_vector_sync_status MCP tool + VectorSyncStatusResponse: add
indexed_documents (distinct) and indexed_chunks; keep indexed_count as a
deprecated alias of indexed_chunks. Reuses count_indexed.
- userinfo HTML page (/app/vector-sync/status): show Indexed Documents AND
Indexed Chunks rows; switch its count to count_indexed (which also excludes
placeholder points — the old raw count included them).
- /api/v1/vector-sync/status: restore indexed_count as a deprecated alias of
indexed_chunks so existing consumers (integration tests, pre-#115 UI) keep
working; the change is now purely additive for indexed_count.
Tests: VectorSyncStatusResponse documents/chunks/alias + zeroed defaults.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Rename the lifespan-local `transport` to `ingest_transport` in both paths so it
no longer shadows the get_app(transport=...) HTTP-transport parameter.
- Log the memory backend selection in build_transport, symmetric with the
postgres branch, so startup logs name the chosen ingest backend either way.
- Note in _wire_vector_sync_state why eviction_task_group is intentionally not
set there (it only exists once the lifespan's task group is running).
Refs: Deck #196
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add IngestTransport.active_consumer_count (0 by default; LocalTransport stores
the started count) so app.py logs the worker count without re-checking
INGEST_QUEUE — the last backend-knowledge leak in the lifespan is gone.
- Document that DistributedTransport is postgres/procrastinate-specific by design
(aclose() calls ProcrastinateTaskProducer.drain()); other distributed backends
would be separate IngestTransport subclasses.
- Clarify the _wire_vector_sync_state log line (writes app.state + singleton, not
only the singleton).
- Strengthen the LocalTransport test: assert active_consumer_count transitions
0→N and that each worker receives a distinct cloned receive stream.
Refs: Deck #196
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Note at both metrics-task call sites that receive_stream is None in postgres
mode (get_ingest_pending falls back to procrastinate counts).
- Add test_default_is_exact_true covering the status-endpoint count path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Finish the hexagonal ports-&-adapters split started in #183. The producer side
already had a TaskProducer port + adapters, but the consumer side was
unabstracted and the INGEST_QUEUE selection leaked into a duplicated
`if use_postgres:` branch across both app.py lifespan paths.
Introduce an IngestTransport ABC (vector/queue/transport.py) that bundles the
producer with running (or not running) the in-process consumer pool, built by a
single build_transport() factory:
- LocalTransport (INGEST_QUEUE=memory): in-process anyio stream drained by an
N-worker pool that run_consumers starts.
- DistributedTransport (INGEST_QUEUE=postgres): wraps ProcrastinateTaskProducer;
run_consumers is a no-op because the consumer is the external `worker` role.
Both lifespan paths now call build_transport + _wire_vector_sync_state (new
helper that centralizes the app.state / module-singleton / browser-app writes) +
transport.run_consumers + transport.aclose(), with no INGEST_QUEUE branching and
no getattr drain probe. Adding a future backend (Redis/NATS/SQS) is one new
adapter + one build_transport arm, with no app.py or scanner change.
Preserves the single-tenant parallelism invariant (one shared multiplexed queue
+ N-worker pool, per-document not per-user dispatch) and documents it in
ADR-028. The worker CLI is unchanged (it is the external consumer).
Refs: Deck #196 (Deck #197 tracks the explicit parallelism regression test)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add Validator("VECTOR_SYNC_METRICS_REFRESH_INTERVAL", gte=1) so a 0/negative
value can't turn the publish loop into a busy-spin.
- Annotate count_indexed's qdrant_client param as AsyncQdrantClient.
- Add tests: exact kwarg is forwarded to qdrant count, and the placeholder
filter matches False (excludes placeholders) with chunk_index pinned to 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The only queue metric, mcp_vector_sync_queue_size, was updated inline by the
single-user consumer (processor_task) but never by the multi-user consumer
(oauth_processor_task). On multi-user tenants (e.g. blackbox-demo, 5 users) the
gauge read 0 for 24h while the live anyio buffer held ~2214 pending documents
(shown by /api/v1/vector-sync/status). The "indexed" figure was also a chunk
count (16039 points ≈ 480 docs) mislabelled as documents.
Publish a consumer-independent snapshot from a periodic task
(vector/metrics_publisher.vector_sync_metrics_task), spawned in BOTH lifespan
task groups (single-user and multi-user) and every queue backend:
- mcp_vector_sync_pending_documents — outstanding work via
ingest_status.get_ingest_pending() (anyio buffer depth or procrastinate
todo+doing); also keeps the legacy queue_size gauge meaningful on all paths.
- mcp_vector_sync_indexed_documents — distinct documents, counted exactly and
cheaply via the one chunk_index=0 point per document (no facet).
- mcp_vector_sync_indexed_chunks — total non-placeholder points.
The /api/v1/vector-sync/status endpoint now returns indexed_documents (distinct
docs) AND indexed_chunks separately, so documents and chunks are no longer
conflated. The publisher uses approximate Qdrant counts (every-N-seconds gauge);
the on-demand endpoint counts exactly. New knob:
VECTOR_SYNC_METRICS_REFRESH_INTERVAL (default 20s). Fail-safe: a metrics refresh
never disturbs ingest.
BREAKING CHANGE: /api/v1/vector-sync/status field `indexed_documents` now holds
the distinct-document count (was the chunk count); the chunk count moved to the
new `indexed_chunks` field. The Astrolabe UI + the nc_get_vector_sync_status MCP
tool / userinfo page are harmonized in a follow-up (Deck #195).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Optional review hardening on #849 (non-blocking nits from the approve):
- `_build_search_xml`: emit `<d:firstresult>` on `offset is not None` rather
than truthiness, so a future explicit offset=0 isn't silently dropped.
- `_key`: key on `file_id is not None` so a (hypothetical) file_id of 0 isn't
treated as absent and mis-keyed onto path.
- `_type_search_args`: XML-escape the MIME type before interpolating it into
the SEARCH literal (defense-in-depth for any future user-supplied value),
with a unit test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review on #849:
- Critical: add missing `await` on the exception-path fallback in
search_files_all -- it returned a coroutine instead of the result list.
Add unit tests for both the offset-page-raises (fallback) and
offset-zero-raises (propagate) paths, which previously had no coverage.
- Guard `_key` dedup against items missing both file_id and path (fall back
to id(item)) so they can't collapse under a shared None key and drop rows.
- Document the offset-ignored discard-and-refetch decision.
- Split the offset paging into `_search_offset_paged` (returns None to signal
fallback) and share the truncation warning via `_warn_if_truncated`,
cutting cognitive complexity below the threshold (SonarCloud S3776).
- Make the test side_effect helpers synchronous (SonarCloud S7503).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a comment at the deletion-tracking scroll noting that a user who
gained access to a shared file via the tenant-wide dedup path (without
indexing it) is absent from the user_id-filtered indexed_file_ids, so the
grace-period sweep never enqueues a delete for them — their stale
acl_principals entry is reclaimed lazily by verify-on-read eviction.
Addresses review nit #3.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The vector-sync scanner expanded a tagged folder into its PDF descendants via
`WebdavClient.find_by_type(scope=dir)` with no result limit. A WebDAV SEARCH
with no `<d:nresults>` returns only Nextcloud's default page (~100 on the
affected instance), so large tagged folders were silently truncated and most
documents were never queued for indexing (e.g. a 220-file folder yielded 100).
Add `search_files_all`, which pages the SEARCH to completion. It uses
`<d:firstresult>` offset paging where supported and, because Nextcloud 31
ignores offset (verified against a live instance), detects the repeated page
and falls back to a single bounded fetch with an explicit large `<d:nresults>`.
`find_all_by_type` wraps this and is now used for tagged-folder expansion;
`find_by_type` is unchanged for the interactive MCP tools.
Crossing `WEBDAV_SEARCH_MAX_RESULTS` logs a warning and increments the new
`astrolabe_document_scan_truncated_total` metric, so a coverage cap can never
again hide files silently.
Scope: this fixes discovery only. Cross-user double-processing of identical
shared files (point-ID collisions) is tracked separately.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
existing_principals() ran for every doc type when seeding acl_principals.
note/news_item/deck_card IDs are per-user (not globally unique) and chunk
point IDs are user-agnostic, so on an ID collision the merge would pull in
another user's principal and cross-surface their content via the
acl_principals search branch. It was also N wasted tenant-wide scrolls on
initial sync for those types. Gate the prior-principal merge on
doc_type == "file" (the only type with cross-user dedup + globally-unique
fileid); other types seed with the indexer only.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A file shared across many users — directly, or via a group folder shared
to a group — was parsed and embedded once per user. Chunk point IDs are
user-agnostic (uuid5(tenant_id, doc_id=fileid, chunk_index)), but the
per-user freshness gate filtered Qdrant by user_id, so two readers
ping-ponged: each overwrote the other's points and each kept seeing "not
indexed for me", reprocessing every scan. Production telemetry (note
386945, finding #5) measured identical docs re-processed every few hours
at 7-13s each, with PDF parse ~62% of per-doc cost.
Layer 1 — tenant-wide dedup:
- Thread the scanner's tag-REPORT etag into the file DocumentTask and the
chunk payload; index `etag` as a KEYWORD field.
- vector/sharing_state.find_indexed_content scrolls tenant-wide (no
user_id filter) for a non-placeholder point matching
(doc_id, doc_type, etag), gated on embedding_identity in Python so a
model switch correctly forces a re-embed.
- Scanner skips enqueue and the processor skips fetch/parse/embed when a
match exists (cross-worker race-guard before WebDAV read). Dedup is
fail-safe: a Qdrant error degrades to "process normally".
Layer 2 — observed-access ACL (no admin / GroupFolders API needed):
- Each point carries `acl_principals` = the set of user:<uid> whose
scanner has observed (hence can read) the file. The per-user tag REPORT
is the access oracle; group membership/GroupFolders enumeration is
admin-only and unavailable in multi-user modes.
- build_ownership_filter ORs MatchAny(acl_principals, ["user:<me>"]) so a
deduplicated shared/group-folder point surfaces to every reader;
verify-on-read (_verify_files) remains the precise ACL gate.
- Deletion/eviction become "release one user": drop the principal and
delete the points only when the set empties, so one user untagging a
shared file doesn't evict it for the others. Legacy points without the
field keep the original per-user delete.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An unset INGEST_QUEUE auto-derived "postgres" whenever DATABASE_URL was
PostgreSQL, silently starting the procrastinate ingest worker (schema
migration, reclaim cron, deferred jobs) on every Postgres-backed tenant —
even though none had opted into the api/worker split. Observed on
tenant-blackbox-demo (:0.98.0): ~600 "Deferred 1 job" log lines / 24h.
Resolve an unset INGEST_QUEUE to "memory" (the in-process anyio queue)
regardless of the database backend. procrastinate is now strictly opt-in
via an explicit INGEST_QUEUE=postgres; the existing guard still rejects
postgres against a SQLite DATABASE_URL. Docs + unit test updated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🟡 The `worker` command never called initialize_document_processors(), so a
worker pod with ENABLE_UNSTRUCTURED/TESSERACT/CUSTOM configured silently ran
PyMuPDF-only (only the import-time-registered processor). The always-on API pod
registers them in its lifespan; the worker has its own startup path, so call
initialize_document_processors() there too (before run_worker_async).
🟢 Drop the unused get_database_url monkeypatch in the Postgres integration
fixture (build_app_for_url passes the URL explicitly; only the ssl lookup needs
pinning).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-4 review (non-blocking) items:
- get_procrastinate_conninfo: warn on an empty connect_timeout= value (it falls
back to the 10s default); preserve an explicit connect_timeout=0.
- Document the _doc_queueing_lock user_id invariant (NC rejects ':' in usernames).
- docs/configuration.md: note that `db downgrade` leaves procrastinate's tables
in place and how to drop them on a full teardown.
- reclaim_stalled_ingest_jobs: debug heartbeat log when nothing is stalled.
- Drop the redundant list() wrap in the integration stalled-jobs assertion.
Logging pattern: define a module-level `logger = logging.getLogger(__name__)`
and use it instead of function-local or inline getLogger(__name__) calls
(config.py, config_validators.py, tests/.../test_scope_authorization.py). The
test file's dev-only `scripts.*` import gets a ty: ignore since it resolves via
sys.path at runtime, not as an installed package.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🟡 Document the _doc_queueing_lock ":" delimiter invariant (user_id and the
controlled doc_type enum are colon-free, so the key is collision-safe; a
future doc_type with ":" must not be added).
🟡 API pod no longer opens the procrastinate connector twice on startup: add
ProcrastinateTaskProducer.ensure_schema() (applies the schema on the
already-open pool) and have both lifespan branches build the producer then
ensure_schema — one open/close cycle, matching the worker. build_producer now
returns the concrete producer type.
🟢 Document in ports.py that a long-lived-connection producer may optionally
provide drain() (lifespan probes via getattr).
🟢 Add a unit test that a non-credential pipeline error propagates (for
procrastinate's RetryStrategy) and still closes the client via finally.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🟡 Document why ProcrastinateTaskProducer.connect() uses `await app.open_async()`
(AwaitableContext: await opens a long-lived pool, closed by drain()) and add a
connect()/drain() lifecycle unit test (InMemoryConnector) asserting the pool is
opened by connect and closed by drain — previously untested.
🟡 get_procrastinate_conninfo: forward connect_timeout from DATABASE_URL or
default 10s so an unreachable DB can't hang worker/API startup indefinitely;
warn only on other dropped query params. + tests.
🟢 INGEST_DELETE_SUCCEEDED_JOBS (default true) makes the worker's succeeded-job
deletion configurable for audit retention.
🟢 Worker startup logs via logger.info (structured/OTel) instead of click.echo.
🟢 INGEST_STALLED_JOB_SECONDS (default 300) makes the crash-reclaim threshold
tunable for slow embedding backends; reclaim reads it per-run.
The broad `except` in _apply_ingest_queue_schema_open is kept deliberately:
procrastinate wraps psycopg errors, so narrowing to psycopg.errors.* would miss
the wrapped DDL-conflict and turn a benign concurrent-apply race into a failure;
the presence re-check re-raises genuine errors.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 3 review follow-ups:
- Enforce the folder cap (MAX_PATH_PREFIXES=20) inside normalize_path_prefixes
so the REST/viz endpoints are bounded too, not just the MCP tool's Field
and the PHP client. Single server-side enforcement point; the MCP tool's
Field(max_length=...) now references the same constant.
- Widen the SearchAlgorithm ABC and both concrete implementations'
path_prefixes param to Iterable[str] | None, matching the widening of
build_base_filter_conditions from the prior round.
- Add a normalize_path_prefixes cap test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 2 review follow-ups:
- Add Field(max_length=20) to the nc_semantic_search path_prefixes param so
an LLM client can't build an unbounded OR-filter (mirrors the cap the
Astrolabe PHP controller applies on the UI path).
- Note in normalize_path_prefixes that the two-pass collect-then-strip is
deliberate (the `if path_prefix:` guard is truthy for whitespace-only
input; the strip pass is what drops it).
- Tests: exercise build_base_filter_conditions with 3 folders (guards the
list comprehension) and parametrize the no-path case over None, empty
list, and blank-only inputs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- visualization.py: drop the CSV string-split branch. The Astrolabe PHP
client sends path_prefixes as a JSON array, so only a list is accepted;
any other shape is ignored rather than comma-split (which would corrupt
folder names containing commas).
- viz_routes.py: split the path_prefixes query param on newline (a comma
is a valid POSIX path char; a newline is not) and pass None instead of
[""] when the param is absent.
- access_filter.py: widen build_base_filter_conditions' path_prefixes to
Iterable[str] for consistency with normalize_path_prefixes.
- ADR-027: document the newline delimiter (frontend/viz route) and JSON
array (PHP->MCP body), and the PHP-side cap on list width.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extend the ADR-027 Phase 2 path filter from a single path_prefix to a
list of folders. The new normalize_path_prefixes() helper is the single
source of truth for trimming, dropping blanks, and de-duplicating, and
folds the legacy single path_prefix into the list for backward
compatibility.
build_base_filter_conditions() adds one MatchText to the must clause for
a single folder (unchanged shape) and OR-s multiple folders via a nested
Filter(should=[...]) so a file under any selected folder matches while
still AND-ing against the ACL/doc_type/date conditions.
path_prefixes is threaded through every search surface: the
nc_semantic_search MCP tool, the visualization API (JSON body), and the
viz route (CSV query param). The Astrolabe frontend folder picker that
produces these lists ships in a companion astrolabe PR.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🔴 nc_get_vector_sync_status reported pending=0 for INGEST_QUEUE=postgres: the
AppContext/OAuthAppContext per-session yields snapshotted the stream fields but
never forwarded task_producer, so lifespan_ctx.task_producer was always None.
Convert task_producer to a @property that reads _vector_sync_state live (like
eviction_task_group), removing the snapshot field so the yields can't drop it.
Add a regression test pinning the contract on both contexts.
🟡 Remove the unused _RECLAIM_TASK_NAME constant.
🟡 get_procrastinate_conninfo: warn + document that DATABASE_URL query params
(application_name, connect_timeout, …) are dropped.
🟡 worker: open the procrastinate App once — apply_ingest_queue_schema gains
manage_connection=False so the worker reuses its own open connector instead
of a redundant open/close before run_worker_async.
🟢 Clarify the apply-schema broad-except comment (non-race errors re-raise) and
document the deliberate Any typing in ingest_status.get_ingest_pending.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI runs `uv run --frozen ty check -- nextcloud_mcp_server` and `uv run pytest -m
unit`, which install the default + dev groups but not the `[postgres]` optional
extra. vector/queue/procrastinate.py imports procrastinate at module scope (the
task registration needs App/Blueprint), so without it installed ty fails on
unresolved imports and the procrastinate unit tests fail to collect.
Add procrastinate + psycopg to the dev group (kept in the [postgres] extra for
production opt-in) so dev/CI always type-check and test against them, while
SQLite/personal installs stay free of the Postgres deps. Matches the repo's
optional-DB-driver philosophy.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Stop excluding tests/ from the ty-check pre-commit hook so touched test files
are type-checked. Fix the new ingest tests under the now-active check:
- cast duck-typed JobContext / App test doubles to their declared types;
- narrow the gated Postgres fixture's str | None URL (pytest.skip isn't modelled
as NoReturn by ty).
Pre-existing type issues in untouched test modules are unaffected (the hook
checks only changed files); they'll be cleaned as those files are next touched.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-architect document ingest from the shared NATS-glued document-processor to a
per-tenant, in-process model owned by nextcloud-mcp-server (Deck #183). The MCP
server now owns both sides of ingest:
- Producer (api role): the scanner defers one job per changed document into the
app's Postgres via procrastinate (queueing_lock dedup; no execution lock, so a
crashed worker can't deadlock a doc — Qdrant upserts are idempotent).
- Consumer (worker role): `nextcloud-mcp-server worker` drains the queue and runs
the existing process_document pipeline; a periodic task reclaims jobs orphaned
in `doing` by a crash.
INGEST_QUEUE selects the transport (auto: postgres when DATABASE_URL is Postgres,
else the in-process anyio queue for SQLite/dev). procrastinate manages its own
tables (applied on a fresh DB at startup and by `db upgrade`). The vector-sync
status surface reads job counts from Postgres in postgres mode. procrastinate +
psycopg3 ship in the [postgres] extra; the app's own engine still uses asyncpg
(driver unification is a follow-up handled in the rendered Helm chart).
NATS JetStream, the Postgres-queue stub, the bus status subscriber, and nats-py
are removed.
BREAKING CHANGE: the external-NATS-ingest env vars are removed
(INGEST_MODE, STATUS_BACKEND, INGEST_BUS_URL, INGEST_BUS_NUM_REPLICAS,
FACT_EVENT_EMITTER). Use INGEST_QUEUE (memory|postgres) and the `worker`
command instead. TENANT_ID is retained (no longer NATS-subject-charset-validated).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Prevent concurrent version bumps and spurious releases in the release
pipeline:
- Add a workflow-level concurrency group (cancel-in-progress: false) so
only one bump-version run executes at a time. Concurrent runs have
previously raced to bump the version and push tags, causing release
failures. Subsequent pushes now queue instead of cancelling an
in-flight bump/release.
- Make commitizen the single source of truth for whether a release is
warranted. The previous grep heuristic counted commits matching
feat|fix|docs|refactor|perf|test|build|ci|chore, but commitizen only
bumps for feat/fix/breaking changes. A CI- or docs-only push therefore
set bumped=true and fired release+docker against the old, already
released tag. Now we compare the latest tag before/after running
bump-mcp.sh and only set bumped=true (and emit the new tag) when it
actually changes, so release/docker exit early on non-release pushes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remaining items from the PR #831 Claude review:
- processor span symmetry: add "vector_sync.total_chars" to the sparse
embedding span (already on the dense span) and drop the redundant
"embedding.batch_size" attribute from both spans — it always equalled
vector_sync.chunk_count and would mislead once batching is split.
- metrics: document the deliberate "throughput counts only on full success"
contract in record_document_parse (partial extractions flagged
success=False are counted as a parse-error but never inflate
pages/chars/bytes throughput).
- config: extract _detect_base_provider() -> (family, model) as the single
source of truth for the provider-detection priority chain, shared by
get_embedding_model_name() and get_embedding_provider_family(). Preserves
the intentional gateway asymmetry (only the family method short-circuits).
- base.py: Optional[...] -> PEP 604 `... | None`; drop now-unused import.
Behavior unchanged (get_embedding_* outputs covered by test_config.py).
Refs Deck #175, PR #831.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a path_prefix filter to semantic search, honoured on both the MCP tool and
the dense-only visualization/API paths through the shared filter contract.
- build_base_filter_conditions: append FieldCondition(file_path,
MatchText(path_prefix)) when set. file_path is only on doc_type == "file"
points, so a non-empty path_prefix implicitly restricts to files.
- Promote path_prefix to an explicit keyword param on the SearchAlgorithm ABC
and both algorithms; thread it through nc_semantic_search (blank ⇒ no filter),
the /api/v1 search endpoints, and the viz route.
- Add a file_path TEXT payload index to _PAYLOAD_INDEX_FIELDS (no content
re-index; idempotent startup migration). MatchText tokenizes on server Qdrant
and matches by substring on local/embedded qdrant-client — both serve folder
scoping.
- Update ADR-027 (Phase 2 implemented; readiness table; semantics note). Tests.
Refs ADR-027 Phase 2. Deck #177.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a modified_after/modified_before date-range filter to semantic search,
honoured on both the MCP tool path (BM25HybridSearchAlgorithm) and the
dense-only visualization/API path (SemanticSearchAlgorithm) through one shared
contract.
- Promote modified_after/modified_before to explicit keyword params on the
SearchAlgorithm ABC and both concrete algorithms; factor the shared
placeholder+ownership+doc_type+date filter into
access_filter.build_base_filter_conditions so new filters land in one place.
- nc_semantic_search: accept RFC 3339 / ISO 8601 (or Unix seconds) bounds via
utils.validation.parse_modified_timestamp; Annotated/Field constraints on the
numeric args; explicit McpError guard for after > before. Thread the parsed
bounds through the cross-app and per-doc_type dispatch.
- /api/v1 search endpoints + viz route parse the same formats and 400 on bad or
inverted ranges.
- Add a modified_at INTEGER payload index to _PAYLOAD_INDEX_FIELDS; the
idempotent _ensure_payload_indexes() startup path migrates existing
collections with no content re-index.
- Update ADR-027 to resolve the review feedback (validation placement, shared
algorithm contract, deferral of nc_semantic_search_answer, payload index,
RFC-3339-at-the-boundary rationale). Add unit tests.
Refs ADR-027. Deck #177.
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 blocking + important findings from the claude bot's re-review:
- test (blocking): pin the file verifier's fail-open contract for definitive
403/404 on the tag REPORT, not just transient 503/429. A disabled systemtags
endpoint commonly 403s; unlike the per-access verifiers (where 403/404 = drop),
the batch file verifier must keep all results since the whole set hinges on one
REPORT. Adds _http_error(403)/_http_error(404) to
test_verify_files_tag_fetch_failure_keeps_all and documents the asymmetry.
- docs (important): migration caveat — if vector-index was created as
user_visible=False (manual occ tag:add, or pre-release), an owner's tag won't
surface in a recipient's REPORT and shared-file results are silently dropped
after upgrade. Note that the MCP server's get_or_create_tag defaults to
user_visible=True, and how to verify/fix an existing tag.
- docs (important): note the file verifier's latency scales with both the
Depth:infinity folder expansion and the EXCLUDED_TAGS lookup (~2 WebDAV calls
per excluded tag, fanned out under one slot); suggest lowering
VERIFICATION_CONCURRENCY for large excluded-tag lists / deeply tagged trees.
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>
- Failed deletes no longer bump astrolabe_documents_indexed_total: the outer
except in process_document now gates doc_type on operation != "delete", so a
delete error is counted as processed-error but not as an indexing event.
Added test_failed_delete_is_processed_but_not_indexed.
- registry parse span: pass record_exception=True explicitly (matches
instrument_tool) and add a structured logger.warning on the parse-error path
(processor/tier/byte_size/duration_ms) for a Loki-aggregatable failed-parse
signal.
- test_error_does_not_increment_throughput: snapshot-before/delta pattern
instead of absolute 0.0 (counters are global singletons).
- config: document the deliberate gateway asymmetry between
get_embedding_model_name() (no gateway branch) and
get_embedding_provider_family() (short-circuits on gateway).
- Cleanup in touched scope: narrow `except (HTTPStatusError, Exception)` to
`except Exception` (drop now-unused import); convert registry signatures from
Optional[...] to `... | None`.
Refs Deck #175, PR #831.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer findings:
- Fix double-count of exhausted-retry failures: the inner final-retry branch
and the outer except both recorded a processing error. Consolidate to the
outer handler (single call site); inner branch keeps only the Qdrant-upsert
error metric. Regression test added.
- Deletes are no longer counted as indexing events: the delete success path
drops doc_type so astrolabe_documents_indexed_total is not inflated.
Regression test added.
- Reuse the already-resolved `settings` in _index_document instead of a second
get_settings() call.
- Use explicit `> 0` guards in record_document_parse / record_embedding instead
of truthiness checks.
SonarCloud:
- S1244 (BUG): replace float `==` equality in metric tests with pytest.approx.
- S5332 (hotspot): use https in the gateway-URL test fixture.
- S1192: extract the repeated "vector_sync.chunk_count" span-attribute literal
into a module constant.
Review nit: move the duplicated `_sample` test helper into a shared
`metric_sample` fixture in tests/unit/conftest.py.
Refs Deck #175, PR #831.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
