Commit Graph
140 Commits
Author SHA1 Message Date
Chris CoutinhoandClaude Opus 4.8 1f8b3ba95e fix: resolve startup NameError in vector-sync metrics task
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>
2026-06-04 22:10:40 +02:00
Chris CoutinhoandGitHub d95ed7be68 Merge pull request #850 from cbcoutinho/feat/ingest-pending-documents-metric
feat: backend-agnostic vector-sync gauges (pending / documents / chunks)
2026-06-04 21:27:18 +02:00
Chris CoutinhoandClaude Opus 4.8 bf84db35b4 refactor: address PR #851 review round 5 (ingest transport)
- _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>
2026-06-04 20:49:17 +02:00
Chris CoutinhoandClaude Opus 4.8 c4401af9c6 refactor: address PR #851 review round 4 (ingest transport)
- 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>
2026-06-04 20:42:57 +02:00
Chris CoutinhoandClaude Opus 4.8 c0c52c1b34 refactor: address PR #851 review round 3 (ingest transport)
- 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>
2026-06-04 20:36:58 +02:00
Chris CoutinhoandClaude Opus 4.8 2179e9ddb0 refactor: address PR #851 review round 2 (ingest transport)
- 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>
2026-06-04 20:27:40 +02:00
Chris CoutinhoandClaude Opus 4.8 655d608fb7 refactor: address PR #851 review round 1 (ingest transport)
- 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>
2026-06-04 20:07:19 +02:00
Chris CoutinhoandClaude Opus 4.8 e7bcdb1950 feat: add IngestTransport port for local/distributed ingest backends
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>
2026-06-04 19:43:31 +02:00
Chris CoutinhoandClaude Opus 4.8 fc3e0f28f6 fix: add metrics-interval validator + type/test gaps (review #850)
- 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>
2026-06-04 19:39:46 +02:00
Chris CoutinhoandClaude Opus 4.8 fbe70ecd9c feat: backend-agnostic vector-sync gauges (pending/documents/chunks)
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>
2026-06-04 19:30:10 +02:00
Chris CoutinhoandClaude Opus 4.8 31eb2d4e74 docs: explain pure-claimer eviction path in scanner (review #848)
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>
2026-06-04 13:24:41 +02:00
Chris CoutinhoandClaude Opus 4.8 c3758a0bdf fix: only merge prior acl_principals for files (review #848)
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>
2026-06-04 13:02:43 +02:00
Chris CoutinhoandClaude Opus 4.8 1c93e7286d feat: dedup shared-file parsing/embedding across users in vector sync
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>
2026-06-04 12:55:17 +02:00
Chris CoutinhoandClaude Opus 4.8 704a537847 chore: round-4 polish + standardize on module-level loggers
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>
2026-06-03 15:44:59 +02:00
Chris CoutinhoandClaude Opus 4.8 b10ce15032 fix: address PR #836 round-3 review (lock-key invariant, single open)
🟡 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>
2026-06-03 15:32:16 +02:00
Chris CoutinhoandClaude Opus 4.8 820b98dac1 fix: address PR #836 round-2 review (connect/timeout/observability)
🟡 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>
2026-06-03 15:21:50 +02:00
Chris CoutinhoandClaude Opus 4.8 cfdef3c2c5 fix: address PR #836 review — forward task_producer to MCP contexts + cleanups
🔴 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>
2026-06-03 12:40:50 +02:00
Chris CoutinhoandClaude Opus 4.8 21b7922bac feat: replace NATS ingest with procrastinate Postgres queue (#183)
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>
2026-06-03 04:11:11 +02:00
Chris CoutinhoandGitHub d561b350d1 Merge pull request #831 from cbcoutinho/feat/document-pipeline-observability
feat(observability): astrolabe_* metrics + traces for the document pipeline
2026-06-03 02:08:46 +02:00
Chris CoutinhoandClaude Opus 4.8 b779627fa6 fix(observability): address third review round
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>
2026-06-03 02:01:28 +02:00
Chris CoutinhoandGitHub 9adbf9e8a5 Merge pull request #834 from cbcoutinho/fix/verify-on-read-tag-gate
fix(search): gate verify-on-read file results on vector-index tag membership
2026-06-03 02:00:34 +02:00
Chris CoutinhoandClaude Opus 4.8 ab128bef5b feat(search): ADR-027 Phase 2 — file-path filter
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>
2026-06-03 00:51:12 +02:00
Chris CoutinhoandClaude Opus 4.8 c2c8dc1a08 feat(search): ADR-027 Phase 1 — modified-date range filter
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>
2026-06-03 00:35:20 +02:00
Chris CoutinhoandClaude Opus 4.8 d4dbf01b0a fix(search): gate verify-on-read file results on vector-index tag membership
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>
2026-06-02 21:14:44 +02:00
Chris CoutinhoandClaude Opus 4.8 68c9e20636 fix(observability): address second review round
- 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>
2026-06-02 21:09:50 +02:00
Chris CoutinhoandClaude Opus 4.8 2e49e442f4 fix(observability): address PR review + SonarCloud findings
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>
2026-06-02 18:15:50 +02:00
Chris CoutinhoandClaude Opus 4.8 5d205fcaab feat(observability): astrolabe_* metrics + traces for the document pipeline
Make per-tier bottlenecks in the document-processing pipeline
(scan -> fetch -> parse -> chunk -> embed -> Qdrant upsert) visible via
metrics, traces, and structured logs. Today the document_processors layer
emits only a logger.info line: no metric, no span, and page counts live only
inside a log string. The single processing-duration histogram is unlabeled and
whole-document, so it cannot isolate parse vs embed vs upsert.

New astrolabe_* metric family (distinct from the mcp_* protocol metrics):
- astrolabe_document_parse_{duration_seconds,total} + pages/chars/bytes counters
  recorded at the ProcessorRegistry.process() boundary (covers all current and
  future processors uniformly)
- astrolabe_document_escalation_total (dormant; tiered-pipeline readiness)
- astrolabe_embedding_{duration_seconds,requests_total,chunks_total,chars_total}
- astrolabe_document_chunks_total, astrolabe_documents_indexed_total{source,status}

Tracing: new document_processor.parse child span + enriched embed/chunk span
attributes (provider/model/batch_size/chunk_count). Structured logs gain a
consistent field vocabulary (doc_id, doc_type, processor, tier, pages, chars,
byte_size, chunks, duration_ms, status) so Loki can aggregate without regex.

Tier-readiness: processor/tier are labels from day one and a tier property is
added to DocumentProcessor, so adding docling/OCR/LLM tiers later is additive
(new label values, never new metrics). Tenant comes from the kube namespace
label; mime_type/model are span attributes only (cardinality). Existing
mcp_vector_sync_*/mcp_qdrant_* are left untouched.

Refs Deck #175 (superset of #173 Phase 2). Dashboard/recording-rules follow-up
tracked on #175 for homelab-argocd.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-02 17:46:20 +02:00
Chris CoutinhoandGitHub c9c0a1d5d4 Merge pull request #821 from cbcoutinho/fix/app-password-loginname-runtime-auth
fix(auth): authenticate stored app passwords with loginName, not UID
2026-05-31 22:17:34 +02:00
Chris CoutinhoandClaude Opus 4.8 cd2145df09 fix(vector): make NATS status subscriber resilient at startup
Follow-up to PR #814 review.

NatsStatusSubscriber.run() called task_status.started() *after* the fallible
pull_subscribe, so a NATS broker that wasn't ready when the MCP server started
would crash the lifespan instead of retrying. Bus status is a non-critical
observability path, so:

- signal started() before the first subscribe (semantics: "loop is running",
  not "subscription succeeded");
- retry a failed subscribe with backoff instead of propagating;
- on a real fetch error (not an idle timeout) drop the subscription and
  re-subscribe rather than fetching against a possibly-dead handle.

Also anchor the _content_hash etag-threading TODO to the PR #814 review thread
so it is discoverable outside git blame.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-05-31 21:56:12 +02:00
Chris CoutinhoandClaude Opus 4.8 874c7c109b fix: stop S7632 flagging NOSONAR mentioned in prose comments
SonarCloud's python:S7632 parses the literal ``# NOSONAR`` token wherever it
appears — including inside explanatory comments that *quote* the directive —
and treats the following text as a malformed suppression. The actual bare
``# NOSONAR`` suppression lines are fine; the flagged lines were the prose
comments describing them. Reword those comments to drop the inner ``#`` so the
analyzer no longer sees a directive.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-05-31 21:38:14 +02:00
Chris CoutinhoandClaude Opus 4.8 52da297ded fix(auth): authenticate stored app passwords with loginName, not UID
Nextcloud authenticates app passwords against the *loginName*, which differs
from the UID for OIDC-provisioned users (e.g. user_oidc makes the UID the
display name: UID "Ada Lovelace", loginName "ada@example.com"). The runtime
consumers of stored app passwords bound the UID as the BasicAuth username, so
every Notes/Files/Shares/CalDAV call returned HTTP 401.

PR #818 fixed only the provisioning endpoint; the consuming paths were missed.
Observed on a login_flow tenant (NC's own OIDC app as IdP): the background-sync
scan loop never started ("Credential validation failed ... HTTP 401") and
semantic search returned 0 results because the ACL shared_with_me lookup 401'd
and degraded to a self-only owner filter.

Root cause: NextcloudClient / CalendarClient conflated two identities — the
DAV/URL path identity (the user_id the whole system keys on = NC UID) and the
auth-credential username (the loginName). Decouple them:

- Thread a keyword-only auth_username through NextcloudClient -> CalendarClient
  (defaults to username, so single-user / OAuth where UID == loginName is
  unchanged).
- get_user_client_basic_auth (background sync + the /api/v1/vector-viz/search
  endpoint) authenticates as the stored loginName, UID for paths.
- _get_client_from_login_flow (the get_client(ctx) MCP-tool path) does the same.
- cleanup_invalid_app_passwords validates with the loginName, so it no longer
  401s and wrongly deletes a valid OIDC user's password.

The loginName is already persisted in app_passwords.username and returned by
get_app_password_with_scopes. Adds unit tests covering the UID != loginName
split for both client builders, the calendar credential/path split, and the
cleanup validation. Also genericises the example user in the #818 comment/test
(real name/email -> Ada Lovelace / ada@example.com).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-05-31 21:34:05 +02:00
Chris Coutinho 1528a1248d Merge remote-tracking branch 'origin/master' into feat/decomp-hook-points
# Conflicts:
#	nextcloud_mcp_server/vector/scanner.py
2026-05-31 20:10:37 +02:00
Chris CoutinhoandClaude Opus 4.8 ccfe179a38 fix: well-form NOSONAR suppressions (SonarCloud S7632/S7503)
The new decomposition modules used `# NOSONAR: reason` (colon form), which
SonarCloud flags as a malformed suppression comment (python:S7632) and which
fails to suppress the intended issue. Switch to the repo's bare `# NOSONAR`
convention with the rationale in a comment above, matching config.py and
auth/storage.py. This also lets the suppression silence python:S7503 (async
method without await) on the protocol-required no-op aclose stubs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-05-31 20:04:44 +02:00
Chris CoutinhoandClaude Opus 4.8 2d845cb70f fix: address PR #814 reviewer follow-ups
- gateway_client: guard token cache with a lazy anyio.Lock so concurrent
  embed calls share one M2M token request instead of racing
- status subscriber: distinguish idle fetch timeouts from real broker
  errors (log + 5s backoff) instead of swallowing all and spinning
- nats: warn when the bus URL uses unencrypted transport (non-tls://)
- collection_metadata: accept an optional shared httpx client, make TLS
  verify explicit, document the unauthenticated control-plane contract
- replace python -O-stripped asserts with explicit ValueError in the bus
  status builder and the api metadata source
- document why the nil-UUID sentinel point can't collide with content ids

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-05-31 19:42:09 +02:00
Chris CoutinhoandClaude Opus 4.8 7f40f7fdb3 fix(vector-sync): isolate per-app scans so a disabled Notes app can't abort sync
The Notes scan in scan_user_documents ran inline without a try/except, while
files/news/deck each had their own guard. On instances without the Notes app
installed, notes.get_all_notes() raises HTTPStatusError 404, which propagated
out of scan_user_documents and aborted the entire per-user vector sync before
files/news/deck were ever reached -- yielding "0 documents indexed" and, after
5 consecutive errors, stopping the scanner.

Extract the Notes scan into scan_notes() (mirroring scan_news_items /
scan_deck_cards) and wrap the call in a per-app try/except. A 404 (app not
installed/disabled) is now logged at info and skipped; other apps still scan.
Deletion-tracking runs only after a successful Notes fetch, so a failed fetch
can never mass-delete a user's indexed notes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-05-30 14:34:25 +02:00
Chris CoutinhoandClaude Opus 4.8 b5ed1e3b4d fix: address PR #814 review + SonarCloud gate
SonarCloud:
- Resolve 6 S5332 hotspots (http→https in test fixture URLs).
- S6418: hoist the unauthenticated AsyncOpenAI placeholder to a named constant
  + NOSONAR (genuine non-secret; gateway ignores it when unauthenticated).
- Fix two reliability bugs: None-index guard in the gateway token-cache test
  (S2259) and float `> 0.0` instead of `!= 0.0` in the sentinel test (S1244).
- status.py idle path sleeps 0.1s instead of sleep(0) (S7491); NOSONAR on the
  protocol-required async no-await aclose() stubs (S7503).

Claude review:
- Remove three leftover debug print() calls in app.py (logger.info already
  covers them).
- payload_backfill: drop parsed_at from the backfilled-keys docstring (it is
  per-document state, not a deployment scalar); add a clean 404 precondition
  for BasicAuth deployments without an OAuth token verifier.
- status.py: task_status typed TaskStatus | None (drop type: ignore).
- nats.py: TODO to thread etags for file/deck/news; note etag default → None.
- factory: warn on unknown INGEST_BUS_URL scheme; raise ValueError instead of
  assert for the external-mode preconditions.
- docs/configuration.md: document the decomposition hook-point env vars + that
  nats-py ships core (lazy-imported) and external+bus uses two NATS connections.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-05-29 18:36:42 +02:00
Chris Coutinho a92f6260fb Merge remote-tracking branch 'origin/master' into feat/decomp-hook-points
# Conflicts:
#	nextcloud_mcp_server/vector/scanner.py
2026-05-29 18:31:05 +02:00
Chris CoutinhoandClaude Opus 4.8 063bd06ba6 fix: cache app-password storage to avoid per-request Alembic upgrade race
get_user_client_basic_auth built a fresh RefreshTokenStorage and ran
storage.initialize() — a full Alembic `upgrade` in a worker thread — on every
call. Once the /api/v1 search endpoints (unified_search, vector_search) and the
chunk-context endpoint were wired to use it, concurrent requests ran concurrent
Alembic upgrades, which race on Alembic's non-thread-safe module-global
EnvironmentContext proxy and intermittently raise `KeyError: 'script'` →
HTTP 500 (seen on multi-user-basic/nc31; nc32 got lucky).

Cache one process-wide, already-initialized storage instance behind a lazily
created anyio.Lock so the one-time migration runs exactly once and never races.
Callers passing an explicit `storage` are unaffected. Also removes a redundant
per-request migration from the search hot path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-05-29 17:37:36 +02:00
Chris CoutinhoandClaude Opus 4.8 cafbfd15a9 fix: address PR #813 review (owner_id index, cache bound, explicit param)
- vector/qdrant_client.py: add owner_id to _PAYLOAD_INDEX_FIELDS (BLOCKING).
  Every search applies MatchAny(key="owner_id", ...); without a keyword index
  Qdrant full-scans the collection and may 400 on Qdrant Cloud strict mode.
  _ensure_payload_indexes is idempotent so existing collections migrate at
  startup.
- search/access_filter.py: bound the process-global _owners_cache with an LRU
  cap (was one unbounded entry per active user, never evicted); document the
  owner-level over-fetch limitation (a prolific sharer floods the recall
  buffer with ghost candidates that verify-on-read drops, with no second
  Qdrant pass) as a TODO toward per-file filtering.
- search/algorithms.py + semantic.py + bm25_hybrid.py: promote
  accessible_owners from **kwargs to an explicit keyword-only parameter on the
  SearchAlgorithm ABC and both implementations, so a misspelled keyword is a
  type error rather than a silent fall back to self-only scope.
- search/verification.py: document that _verify_files now verifies by global
  file id (WebDAV SEARCH), not by path.
- tests/unit/search/test_access_filter.py: add cache-hit, TTL-expiry,
  failure-not-cached, and LRU-bound tests.

Bumps the astrolabe submodule with the matching #89 review fixes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-05-29 13:28:45 +02:00
Chris CoutinhoandClaude Opus 4.8 d883052fb8 feat: add opt-in MCP decomposition hook points (design §10)
Adds the seven §10.2 hook-point modules + five env vars so Astrolabe Cloud can
offload document processing to the external document-processor / embedding
gateway. Purely additive: with every setting unset the server behaves exactly
as today, so self-hosters are unaffected (Deck #92).

Hook points (all default to current monolith behavior):
- config: EMBEDDING_PROVIDER, INGEST_MODE, STATUS_BACKEND,
  COLLECTION_METADATA_SOURCE, FACT_EVENT_EMITTER (+ supporting settings),
  validated in Settings.__post_init__ (fail-fast STATUS_BACKEND=local with
  INGEST_MODE=external); shared canonical.py.
- vector/payload_keys.py + acl_hash.py: cross-impl NAMESPACE/point_id (§2.2)
  and BLAKE2b-128 ACL hash (§11), pinned by fixtures shared with the
  document-processor repo.
- embedding/gateway_client.py: OpenAI-compatible GatewayProvider authenticating
  via M2M OIDC client-credentials (separate realm); manual-only registry entry.
- vector/collection_metadata.py: sentinel-point / API metadata source with env
  fallback.
- vector/queue/: hexagonal ingest producer ports + memory/NATS adapters
  (Postgres seam); INGEST_MODE=external publishes mcp.ingest.requested.{tenant}
  instead of the in-memory stream and skips the in-process processor pool. The
  lifespan becomes a composition root across both deployment branches.
- vector/queue/status.py: STATUS_BACKEND=bus subscriber feeding a StatusStore
  the vector-sync status endpoint reads.
- admin/payload_backfill.py: POST /api/v1/admin/payload-backfill (admin scope);
  processor writes the new payload keys; query-side ACL pre-filter gated behind
  ACL_PREFILTER_ENABLED (default off).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-05-29 13:13:25 +02:00
Chris CoutinhoandClaude Opus 4.7 37db82613d feat(search): ACL-aware vector filter via Nextcloud Shares lookup
The vector index has always been strictly per-user: every Qdrant payload
carries a `user_id` and the search filter is `user_id == querying_user`.
A file Alice indexed cannot be discovered by Bob even if she has shared
it with him — Bob would have to re-index it under his own user_id to
make it searchable, which means duplicate index entries for every share
recipient.

Switch to ownership-with-ACL-expansion:

- New `nextcloud_mcp_server.search.access_filter` module:
  - `list_accessible_owners(sharing_client, user_id)` calls the OCS
    Sharing API (`shared_with_me=true`) and returns
    `{user_id} ∪ {uid_owner of each share}`. Fails open to `[user_id]`
    so a misbehaving Sharing API doesn't black-hole search.
  - `build_ownership_filter(user_id, accessible_owners)` returns a
    Qdrant `Filter` whose `should` branch matches either the new
    `owner_id IN accessible_owners` field or the legacy `user_id` field.
    The legacy branch keeps points indexed before this change reachable
    without a migration backfill.
- Indexer payload (`vector/processor.py`) now writes `owner_id` alongside
  `user_id`. `DocumentTask` gains an optional `owner_id` field; today the
  scanner always runs as the owner so the processor falls back to
  `user_id`, but the field is plumbed so a future shared-with-me crawler
  can set the true owner without reshaping the payload contract.
- `SemanticSearchAlgorithm.search` and `BM25HybridSearchAlgorithm.search`
  accept `accessible_owners` via kwargs and use the new ownership filter.
  Default behaviour with no kwarg is unchanged (self-only).
- Both user-facing callers — the MCP tool path (`server/semantic.py`) and
  the visualization Starlette route (`auth/viz_routes.py`) — compute
  `accessible_owners` from the authenticated Nextcloud client before
  invoking the search algorithm. Eviction, scanner deletion, placeholder,
  and chunk-context paths intentionally keep the legacy `user_id`
  semantics (those are "operations on a specific user's records", not
  cross-user reads).
- 10 new unit tests in `tests/unit/search/test_access_filter.py` cover
  self-only default, owner expansion, dedup, fallback fields, OCS
  failure, and the legacy `should`-branch shape.

Pairs with cbcoutinho/astrolabe#89 — together they let an Astrolabe user
find content owners have shared with them without going through any
re-authorization flow or re-indexing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-27 23:48:34 +02:00
Chris CoutinhoandGitHub baf4d4d225 Merge pull request #809 from cbcoutinho/feat/deck-webhook-presets
feat(webhooks): add Deck card sync preset with vector indexing
2026-05-24 12:42:18 +02:00
Chris CoutinhoandClaude Opus 4.7 c03223a41c fix(vector-sync): use resolved collection name in orphan sweep
The startup sweep was reading settings.qdrant_collection (the raw config
value, default "nextcloud_content") instead of settings.get_collection_name(),
which is what every other vector-sync operation uses. When QDRANT_COLLECTION
is not overridden, get_collection_name() auto-generates a
{deployment-id}-{model-name} name; the sweep was targeting a non-existent
collection and silently returning (0, 0).

Also adds the AsyncQdrantClient type annotation that was missing on
sweep_orphan_placeholders, and renames its parameter from collection_name
to collection to make it clear the value must be the resolved name.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 21:29:48 +02:00
Chris CoutinhoandClaude Opus 4.7 a5cbe91b29 fix(vector-sync): sweep placeholder orphans at Pod startup (#101)
When the per-tenant nextcloud-mcp-server Pod OOMKills mid-batch, the
in-memory anyio processor queue is lost but the placeholder Qdrant
points (is_placeholder=true, status=pending) survive. The next Pod's
scanner re-runs, sees the existing placeholders, applies the
5 × VECTOR_SYNC_SCAN_INTERVAL staleness gate (~5h with the deployed
1h scan interval), and skips them. Result: 0 documents indexed for
the duration of the gate after every restart.

Stamps a process-level instance_id (UUID per Pod-process) onto every
placeholder write. A new sweep_orphan_placeholders helper, called
once from starlette_lifespan after the Qdrant client is initialised
and before the scanner / user-manager spawns, scrolls the collection
and deletes any placeholder whose instance_id doesn't match the
current Pod's (including placeholders with no instance_id field —
back-compat for pre-fix Pod versions). The scanner's next cycle
naturally re-creates fresh placeholders and queues work normally;
no DocumentTask reconstruction needed.

Sweep is one-shot at startup, not periodic — the existing staleness
gate still covers same-Pod recovery, and the cross-Pod-restart gap
was the only failure mode. Failure is non-fatal (logged via
vector_sync.orphan_sweep_failed) so a transient Qdrant hiccup at
boot doesn't prevent the scanner from running.

Both lifespan branches (single-user BasicAuth, OAuth / multi-user
BasicAuth) call the sweep via a module-local helper. A new
VECTOR_SYNC_ORPHAN_SWEEP_ENABLED setting (default True) provides
an escape hatch.

Closes Deck #101.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 21:03:23 +02:00
Chris CoutinhoandClaude Opus 4.7 fdcbd7bd3f feat(webhooks): add Deck card sync preset with vector indexing
Nextcloud Deck PR #7910 added IWebhookCompatibleEvent to CardCreated/
Updated/DeletedEvent and BoardUpdatedEvent, so Deck can finally emit
real-time webhooks via core's webhook_listeners app. Wire this into
the existing preset → parser → DocumentTask pipeline that already
backs Notes / Calendar / Tables / Forms / Files sync.

- Add deck_sync preset (app=deck, 4 events) and drop the stale
  "Deck does not support webhooks" comment.
- Teach webhook_parser to convert Deck card events into
  DocumentTask(doc_type=deck_card, operation=index|delete) with
  stack_id metadata. BoardUpdatedEvent logs delivery at INFO and
  returns None — the polling scanner reconciles affected cards.
- Cover three new unit tests for the deck create/delete/board-update
  paths plus symmetric fail-open tests for missing card.id /
  node.id in _parse_deck_event and _parse_file_event.

The astrolabe admin UI auto-discovers the new preset via
filter_presets_by_installed_apps(); no astrolabe-side wiring is
required for it to appear in the Webhook Management card grid.

Note: requires Deck ≥1.18.x (where PR #7910 lands); the preset is
hidden when the Deck app isn't installed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-21 09:03:59 +02:00
Chris CoutinhoandClaude Opus 4.7 e0b7afb4b1 fix(embedding): instantiate BM25 singleton off the event loop
The ``BM25SparseEmbeddingProvider.__init__`` calls
``fastembed.SparseTextEmbedding(model_name="Qdrant/bm25")`` which
downloads ~50 MB of model weights from HuggingFace and loads them
into memory — observed >5 s wall-clock in production. The inference
methods (``encode_async``, ``encode_batch_async``) already wrap work
in ``anyio.to_thread.run_sync``, so the design intent is clearly to
keep FastEmbed off the event loop. That protection just didn't
cover the constructor.

Symptom in the Astrolabe Cloud per-tenant deploy (deck #102 smoke):
~30–90 s after a user enables semantic search, the pod tips into a
SIGKILL-restart cycle. Loki shows a single log line

  Initializing BM25 sparse embedding provider: Qdrant/bm25

followed by nothing else from the event loop until exitCode 137.
Kubernetes ``/health/live`` httpGet probe timeout=5s fires 6 times
in a row, kubelet kills the container, restart, repeat.

Fix: switch ``get_bm25_service()`` to an async accessor that wraps
the first-time construction in ``anyio.to_thread.run_sync``. Two
existing call sites (``vector/processor.py:603``,
``search/bm25_hybrid.py:123``) update to ``await``. Both are
already inside async functions so the await is free.

New unit test pins the invariant by monkey-patching
``BM25SparseEmbeddingProvider.__init__`` with ``time.sleep(1)`` and
asserting a concurrent ``anyio.sleep(0.05)`` finishes promptly —
the test fails if the constructor ever runs back on the event loop.

Same pattern exists in ``OllamaEmbeddingProvider.__init__`` (sync
``httpx.get`` health-check). Ollama isn't enabled in any current
deploy; filed as a follow-up.

Refs:
- Astrolabe Cloud deck card #102 (smoke discovery)
- Sibling fix #799 (NullPool for cross-loop-asyncpg, same class
  of "anyio bites you in production" bug)

Verified:
- ``uv run pytest tests/unit/`` — 1027 passed
- ``uv run ruff check`` clean on touched files
- ``uv run ty check`` clean on touched files
- New tests pass

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-17 19:33:24 +02:00
Chris Coutinho 55ca44c9ec Merge remote-tracking branch 'origin/master' into chore/lazy-logging-g004-sweep
# Conflicts:
#	nextcloud_mcp_server/vector/oauth_sync.py
2026-05-13 01:40:32 +02:00
Chris CoutinhoandClaude Opus 4.7 65345fd6eb refactor: drop OAuth-refresh background-sync path from oauth_sync.py
Follow-up to #787/#789 (ADR-022 cleanup). After
`oauth_enabled ↔ enable_login_flow` became an invariant, the
`use_basic_auth=False` branch in `vector/oauth_sync.py` — and the
parameter wiring that fed it — was no longer reachable from any
supported deployment mode. This commit removes the dead code.

- nextcloud_mcp_server/vector/oauth_sync.py:
  - Deleted `get_user_client_oauth` (the OAuth-token refresh helper) and
    its `VECTOR_SYNC_SCOPES` constant.
  - Deleted the `get_user_client` dispatcher. Internal callers now call
    `get_user_client_basic_auth` directly.
  - Dropped the `use_basic_auth: bool` parameter from `user_scanner_task`,
    `multi_user_processor_task`, `_run_user_scanner_with_scope`, and
    `user_manager_task`.
  - Dropped the `token_broker` parameter from the same four functions —
    they no longer need it now that the OAuth-refresh path is gone. The
    `TokenBrokerService` constructed in `app.py` is still used by the
    management API revoke endpoint, just not by background sync.
  - Simplified the user-list query in `user_manager_task` to always read
    from the `app_passwords` table.
  - Replaced all `mode_label = "BasicAuth" if use_basic_auth else "OAuth"`
    with a literal `[BasicAuth]` log prefix (keeps existing log filters
    working).
  - Updated the module docstring to describe the post-cleanup shape.
  - Dropped the now-unused `TYPE_CHECKING` import of `TokenBrokerService`.

- nextcloud_mcp_server/app.py: dropped the `use_basic_auth = True` block
  and the now-stale `token_broker if not use_basic_auth else None` /
  `use_basic_auth` positional args from the two `tg.start(...)` calls in
  the multi-user vector-sync lifespan. Token broker construction stays —
  still consumed by the management API revoke endpoint via
  `app.state.oauth_context["token_broker"]`.

- tests/integration/test_app_password_provisioning.py: deleted four tests
  that exercised the now-removed OAuth-refresh path
  (`test_oauth_mode_uses_refresh_token_only`,
  `test_oauth_mode_raises_error_without_token`,
  `test_get_user_client_oauth_function`,
  `test_oauth_mode_requires_token_broker`) plus the
  `test_get_user_client_dispatches_to_basic_auth` test for the deleted
  dispatcher. Updated the module docstring + imports accordingly. The
  BasicAuth-mode tests (`test_basic_auth_mode_uses_local_storage`,
  `test_multiple_users_basic_auth_mode`, etc.) all remain.

No runtime-behaviour change in any supported deployment mode — the deleted
branches were already unreachable post-PR #787. 3 files changed,
+59 / -301; 1010 unit tests pass; integration jobs for
`mcp-login-flow` and `mcp-multi-user-basic` are the critical regression
gates before merge.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-13 01:13:53 +02:00
Chris CoutinhoandClaude Opus 4.7 665cb9b1eb refactor: convert f-string logging to lazy %-style format (G004)
Sweep all 1676 G004 violations across 112 files, converting
`logger.<level>(f"…{x}…")` to `logger.<level>("…%s…", x)`.

Why: ruff rule G004 was added to pyproject.toml to enforce lazy
%-style logging — defers formatting until the log level is enabled
and lets structured log tooling match the unformatted template.

Conversion preserves rendered output byte-for-byte:
- `{x}` → `%s` + `x`
- `{x!r}` / `{x!s}` / `{x!a}` → `%r` / `%s` / `%a`
- Format specs (`{x:.2f}`, `{x:>10}`) → `%s` + `format(x, 'spec')`
  (printf-style specs aren't 1:1 with Python format specs, so we
  delegate to `format()` to keep identical output)
- Literal `%` → `%%`
- Concatenated f-strings (`f"a {x} " "b"`) flattened
- Trailing kwargs (`exc_info=True`) preserved

Verified:
- `uv run ruff check --select G004` → 0 violations
- `uv run ty check -- nextcloud_mcp_server` → passes
- `uv run pytest tests/unit/` → 1010 passed

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-13 01:12:17 +02:00
Chris CoutinhoandClaude Opus 4.7 282c245da1 refactor(config)!: drop ENABLE_MULTI_USER_BASIC_AUTH env var, fail loud on legacy aliases
Same pattern as the ENABLE_LOGIN_FLOW removal in the previous commit:
the deployment mode (MCP_DEPLOYMENT_MODE) is the single source of truth
for selecting an auth flow. The ENABLE_MULTI_USER_BASIC_AUTH env-var
alias is redundant with `MCP_DEPLOYMENT_MODE=multi_user_basic`.

Unlike the ENABLE_LOGIN_FLOW removal — where silent removal was safe
because Login Flow v2 is the auto-detection default — silent removal
here would be a surprise: a user with only ENABLE_MULTI_USER_BASIC_AUTH=true
in their .env would auto-detect into LOGIN_FLOW after upgrade (wrong
runtime mode). Mitigation: detect_auth_mode now reads os.environ
directly for both legacy aliases and raises ValueError with a one-line
migration message if either is set. Applied retroactively to
ENABLE_LOGIN_FLOW as well — loud is better than silent.

- nextcloud_mcp_server/config.py:
  - Drop the dynaconf env-var alias entry for ENABLE_MULTI_USER_BASIC_AUTH.
  - Update the `enable_multi_user_basic_auth` field docstring to mark it
    as derived / not user-settable.
  - `_is_multi_user_mode()` (early-config helper, runs before Settings
    is built) switched to checking MCP_DEPLOYMENT_MODE directly. Now
    consistent with the canonical detection in detect_auth_mode.
- nextcloud_mcp_server/config_validators.py:
  - Drop the auto-detection branch (`if settings.enable_multi_user_basic_auth`).
    Selection of MULTI_USER_BASIC is now exclusively via the explicit
    MCP_DEPLOYMENT_MODE branch.
  - Add `enable_multi_user_basic_auth` to `_sync_derived_flags` alongside
    `enable_login_flow` — both flags are now derived from the resolved mode.
  - Drop `enable_multi_user_basic_auth` from
    `MODE_REQUIREMENTS[MULTI_USER_BASIC].required` and from the
    `forbidden` lists of SINGLE_USER_BASIC and LOGIN_FLOW (no longer
    user input → no meaningful forbidden check).
  - Add loud-deprecation `ValueError` block at the top of detect_auth_mode
    that errors with a clear migration message when ENABLE_MULTI_USER_BASIC_AUTH
    or ENABLE_LOGIN_FLOW is found in os.environ.
- tests/unit/test_config_validators.py:
  - Switch ~10 fixtures from `enable_multi_user_basic_auth=True` to
    `deployment_mode="multi_user_basic"` (mirrors `enable_login_flow`
    treatment from the previous commit).
  - Switch two `patch.dict(os.environ, {"ENABLE_MULTI_USER_BASIC_AUTH": "true"})`
    blocks to use MCP_DEPLOYMENT_MODE.
  - Rename `test_forbidden_multi_user_basic_auth` to
    `test_forbidden_multi_user_basic_when_credentials_present` — the
    scenario is now an explicit-mode + credentials conflict, not an
    env-var-flag conflict.
  - Add `test_legacy_enable_multi_user_basic_auth_env_var_errors` and
    `test_legacy_enable_login_flow_env_var_errors` to exercise the new
    loud-deprecation ValueError path.
- docker-compose.yml: mcp-multi-user-basic profile switched to
  `MCP_DEPLOYMENT_MODE=multi_user_basic`.
- env.sample: replaced `#ENABLE_MULTI_USER_BASIC_AUTH=true` example with
  `#MCP_DEPLOYMENT_MODE=multi_user_basic`.
- docs/authentication.md, configuration.md, troubleshooting.md,
  auth-flows.md, webhook-management-guide.md,
  configuration-migration-v2.md, ADR-025: replaced env-var examples
  with the canonical MCP_DEPLOYMENT_MODE form.
- docs/ADR-020: marked partly superseded by ADR-022.
- CLAUDE.md: Multi-User BasicAuth section updated to set
  MCP_DEPLOYMENT_MODE.
- nextcloud_mcp_server/vector/oauth_sync.py: module docstring updated.

BREAKING CHANGE: ENABLE_MULTI_USER_BASIC_AUTH is no longer read from
the environment, and setting it now raises a startup ValueError with
a migration message. Replace `ENABLE_MULTI_USER_BASIC_AUTH=true` with
`MCP_DEPLOYMENT_MODE=multi_user_basic`. The same loud-deprecation
check is also applied to the recently-removed ENABLE_LOGIN_FLOW —
replace with `MCP_DEPLOYMENT_MODE=login_flow` (or drop both;
`login_flow` is the auto-detect default when no other auth env vars
are set).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-12 20:06:16 +02:00