- 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>
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>
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 PR #825 review round 2:
- _validate_nextcloud_credentials now only maps OCS HTTP 401/403 to a 401
"invalid credential"; any other non-200 (5xx, 503 maintenance mode) surfaces
as 502 "Nextcloud returned a server error" so ops don't chase a phantom bad
password when Nextcloud is actually down.
- The client-facing 401 message is now a parameter, so delete_app_password keeps
its "Invalid credentials" wording without unwrapping/rebuilding the helper's
JSONResponse.
- Body parsing catches (ValueError, UnicodeDecodeError) instead of bare
Exception, and guards body.get behind isinstance(body, dict) — no longer
swallows RuntimeError/AttributeError or a non-object JSON body.
- Add a unit test asserting 500/503 -> 502.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review of PR #825 surfaced an auth bypass introduced by adding loginName
support to delete_app_password: with the OCS-resolved UID discarded, a user
could authenticate as their own loginName (via the request body) while
targeting another user's path and delete the victim's stored app password.
Add the same UID-mismatch guard provisioning already has, so the
authenticated account must own the path UID (403 otherwise).
Also:
- integration test: build the BasicAuth header via base64 instead of
httpx.BasicAuth._auth_header (private attribute); mark the throwaway test
credential NOSONAR(S2068).
- unit tests: cover the httpx.RequestError -> 502 branch, the standard OCS v2
success shape (meta.statuscode 200), and the cross-user delete 403 guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
provision_app_password validated credentials against OCS v1
(/ocs/v1.php/cloud/user), which always returns HTTP 200 — even on auth
failure, where the real status lives in ocs.meta.statuscode (997) and
ocs.data comes back as an empty list []. The status_code != 200 guard
therefore never fired, execution fell through to [].get("id"), and the
resulting AttributeError escaped as an unhandled 500. This blocked
background vector indexing for any user whose supplied loginName didn't
resolve (e.g. display name "Admin" vs loginName "admin").
Extract a shared _validate_nextcloud_credentials helper that:
- queries OCS v2 (/ocs/v2.php), which maps the OCS status onto the HTTP
status, so a failed credential is a real 401;
- parses the payload defensively (isinstance guards) so a non-dict
ocs.data can never raise;
- returns a clean 502 for an unreachable Nextcloud or a non-JSON body.
delete_app_password shared the same v1.php dead-guard bug, which made its
credential check a no-op (any valid-format password passed) — an auth
bypass on deletion. Route it through the same helper and accept the
loginName from the request body (mirroring provisioning) so OIDC users
whose UID differs from their loginName are not regressed.
Adds unit regression tests for the OCS failure payload, non-dict data,
and non-JSON response, plus a login-flow integration test that provisions
with capitalized ("Admin") and spaced ("Test User") loginNames and asserts
a 401 rather than a 500.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
provision_app_password validated the supplied app password by calling the
OCS cloud/user endpoint with BasicAuth as the *path user_id* (the UID).
Nextcloud keys app-password BasicAuth on the loginName, which differs from
the UID for OIDC-provisioned accounts whose UID is their display name
(UID "Chris Coutinho", loginName "chris@coutinho.io"). Authenticating as
the UID is rejected with HTTP 401 ("App password validation failed"), so
provisioning never completes.
Parse the request body up front and authenticate the OCS validation as the
body's `username` (the Nextcloud loginName), falling back to the path
user_id for legacy callers where UID == loginName. The OCS-returned account
id is still checked against the path user_id (the UID), and the password is
still stored keyed by UID with the loginName alongside.
Note this is not an encoding issue: BasicAuth places the user-id literally
in the header (RFC 7617, no URL-encoding); %20/+/literal-space forms of the
UID all fail — only the loginName authenticates.
Adds a regression test asserting the OCS BasicAuth uses the loginName while
storage is keyed by the UID, plus a backward-compat assertion that callers
without a loginName fall back to the UID.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🟡 Performance: unified_search's _execute sorted but did not cap the merged
multi-doc_type pool, so N doc_types each fetched at search_limit sent
N*search_limit candidates into verify-on-read (one Nextcloud round-trip each).
Cap to search_limit*2 after the sort, matching vector_search, nc_semantic_search
and the viz_routes pattern — bounding verification cost to O(2*search_limit)
regardless of how many doc_types are requested.
🟡 Consistency: _get_deck_metadata_from_qdrant is the one internal Qdrant lookup
that uses a raw user_id filter instead of build_ownership_filter. This is not a
bug — deck cards are a documented cross-user gap (the Deck API is per-user, so
cross-user context can't be fetched with the caller's credentials) — but the
inconsistency was unexplained. Added a comment documenting the deliberate
self-only scope.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🟡 Important: nc_semantic_search's include_context branch did not forward
accessible_owners to get_chunk_with_context, so context expansion for shared
files stayed self-only, found nothing in Qdrant, and silently fell back to the
plain excerpt. Forward accessible_owners (the per-file file_accessible_by_id
gate still enforces access).
🟡 Performance: auth/viz_routes.py's multi-doc_type branch sorted but did not
cap the candidate pool before verify-on-read, so N doc_types × limit*2 went
into verification (N× the Nextcloud round-trips). Cap to limit*2 after the
sort, matching server/semantic.py and the cross-app branch.
Also clear the SonarCloud gate (new_duplicated_lines_density 5.1% > 3%) the
ACL wiring introduced: extract the duplicated /api/v1 client-resolution +
owner-expansion + verify-on-read block from unified_search/vector_search into a
shared _search_with_acl helper, define a constant for the repeated
"Nextcloud host not configured" literal (S1192), and reword the access_filter
move_to_end comment so it isn't misread as commented-out code (S125) while
adding the other-owner count to its debug log (review nits).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1. Don't log unverified result titles: both search algorithms logged top-5
titles at DEBUG before verify-on-read; with owner-level share expansion the
unverified set can contain other users' docs. Algorithms now log a count
only; the verifying callers (server/semantic, viz_routes, api/visualization)
log verified titles after verify-on-read.
2. Cross-user FILE chunk context: get_chunk_with_context + the Qdrant chunk
helpers now take accessible_owners and use build_ownership_filter. For files
the expanded scope is honoured only after a per-file file_accessible_by_id
check (accessible_owners is owner-level, so the gate prevents a one-file
share recipient from reading any of the owner's cached chunks). note/deck/
news stay self-only (per-user APIs) — a documented gap. Both chunk endpoints
pass accessible_owners.
3. Algorithm usage: SemanticSearchAlgorithm is not dead (it backs the dense-only
option on the viz/API surfaces); added a clarifying comment in server/
semantic.py. Additionally wired accessible_owners + verify-on-read into the
/api/v1 search routes (unified_search, vector_search) so the astrolabe
surface is ACL-aware too — degrading gracefully to self-only/unverified for
non-provisioned callers instead of 401.
4. Overlapping conditions: build_ownership_filter no longer lists self in the
owner_id MatchAny branch (self is already covered by the user_id branch);
the owner_id branch carries only the OTHER owners.
Tests: build_ownership_filter dedup + chunk-bbox filter-shape updates; new
ACL-aware get_indexed_doc_types, cached-chunk lookup, and end-to-end cross-user
file chunk-context (recipient gets the chunk, non-recipient denied) tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
Two fixes surfaced while testing Login Flow v2 provisioning behind a split
internal/external host (Docker: server↔Nextcloud over http://app, browser
over http://localhost:8080):
1. login_url pointed at the internal host. Nextcloud builds the login URL
from the request host, so the browser-facing URL came back as
http://app/login/v2/flow/... — unreachable from the user's browser. The
poll endpoint was already rewritten to the internal host (correct, the
server polls it); now LoginFlowV2Client also rewrites the login_url origin
to settings.nextcloud_public_issuer_url when set (passed at all 5
construction sites). When unset, behaviour is unchanged.
2. The app-password format guard rejected raw session tokens. core/
getapppassword returns a long alphanumeric token, not the dashed 25-char
Security-settings format, so the dashed-only regex 400'd the one-click
opt-in handoff. Relax APP_PASSWORD_PATTERN to `^[a-zA-Z0-9-]{20,256}$`;
the authoritative validation is still the BasicAuth check against Nextcloud.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The AuthMode.OAUTH_SINGLE_AUDIENCE enum was a vestige of ADR-021's
original design where it co-existed with OAUTH_TOKEN_EXCHANGE. The
un-augmented OAuth bearer pass-through it represented relied on
Nextcloud-side patches to user_oidc (Bearer token validation on
non-OCS endpoints) that were never merged upstream (see
docs/authentication.md, docs/login-flow-v2.md). The working path —
mcp-login-flow profile — sets ENABLE_LOGIN_FLOW=true on top of this
mode so Login Flow v2 acquires per-user Nextcloud app passwords via
a browser flow. With OAUTH_TOKEN_EXCHANGE removed in 57303135, the
_AUDIENCE suffix in the Python name no longer disambiguates anything,
and the enum value diverged from the env-var spelling. ADR-022 (now
accepted) called for this rename as step 1 of consolidation.
- nextcloud_mcp_server/config_validators.py: rename enum to LOGIN_FLOW
with value "login_flow". The mode_map key is now "login_flow"; the
MODE_REQUIREMENTS entry requires `enable_login_flow=True`. Added a
validation gate so MCP_DEPLOYMENT_MODE=login_flow without
ENABLE_LOGIN_FLOW=true errors with a clear message pointing at
ADR-022. Default auto-detection fallback returns LOGIN_FLOW.
- nextcloud_mcp_server/app.py: renamed three identifier references and
switched the "Configuring MCP server for OAuth mode" log line to
the uniform `mode.value` shape used by the other modes.
- nextcloud_mcp_server/api/management.py: renamed identifier in the
/api/v1/status mapping. The user-visible "auth_mode": "oauth" string
is preserved — that's a stable Astrolabe contract.
- nextcloud_mcp_server/config.py: updated Settings docstring.
- tests/unit/test_config_validators.py: renamed class
TestOAuthSingleAudienceValidation → TestLoginFlowValidation,
individual test methods, env-var strings; added enable_login_flow=True
to fixtures expecting success; added a new test
(test_login_flow_requires_enable_login_flow_flag) that exercises the
validation gate.
- tests/unit/test_management_status_endpoint.py: renamed identifier.
BREAKING CHANGE: MCP_DEPLOYMENT_MODE=oauth_single_audience is no longer
accepted. Set MCP_DEPLOYMENT_MODE=login_flow (and keep
ENABLE_LOGIN_FLOW=true) for the same deployment. The un-augmented
OAuth path is no longer supported; if you previously ran the broken
path, you can either configure Login Flow v2 (recommended) or switch
to multi_user_basic / single_user_basic.
Dead-code pruning of `oauth_enabled and not enable_login_flow`
branches in app.py (lifespan, background sync) is deferred to a
separate follow-up PR per the consolidation plan in ADR-022.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Detect pre-existing payload indexes with the wrong schema type in
`_ensure_payload_indexes`. The previous "field already in
existing_schema → skip" branch silently survived a collection migrated
from the int-doc_id era where `doc_id` is indexed as INTEGER, letting
`MatchValue(value="123")` searches keep failing with HTTP 400 on Qdrant
Cloud strict mode — exactly the production failure this PR was meant to
fix. New behaviour: compare `existing_schema[field].data_type` against
the declared type; on mismatch log a WARNING and append to
`failed_fields` so the consolidated end-of-function summary picks it up.
No auto-repair (operator intervention only — see docs/configuration.md
recovery procedure). New test exercises the doc_id-INTEGER scenario
end-to-end and asserts both the per-field WARNING and the summary line.
Clarify the `_verify_news_items` malformed-doc_id rationale: the news
API has no per-item endpoint, so a malformed doc_id genuinely cannot be
verified against the source of truth. We err toward false-positive
(keep) over false-negative (drop) — same conservative posture as
`_verify_notes` and `_verify_deck_cards`. The producer-side validation
is the real security boundary; the verifier is defence-in-depth. Both
the inline comment and the WARNING message now spell this out.
Add a TODO in `get_last_indexed_timestamp` flagging the O(N) cost on
every incremental sync tick. The previous single-page `limit=10_000`
silently bounded the scroll; paginating fixed correctness but made the
unbounded cost visible. The follow-up tracker (canonical TODO at
`api/visualization.py`) covers migrating the max-`indexed_at` to a
sentinel point or collection metadata for O(1) lookup.
Consolidate the duplicate non-numeric-doc_type TODOs at
`api/visualization.py:508` and `auth/viz_routes.py:570` into a single
canonical comment in `visualization.py`; `viz_routes.py` is reduced to
a back-reference. Removes the rot risk of "fixed in one place,
forgotten in the other." The canonical comment also references the
O(1) timestamp follow-up in `scanner.py`.
Document the `batch_size = 256` (qdrant_client.py) vs
`_DELETION_TRACKING_PAGE_SIZE = 1024` (scanner.py) split with
cross-referencing comments at each site: the smaller batch is for the
read-write backfill upsert path (Qdrant accepts ~256-point chunks
comfortably); the larger page is for read-only deletion-tracking
scrolls where no per-page write round-trip applies.
Replace `assert qdrant_client is not None` in `scan_user_documents`
with `cast(AsyncQdrantClient, qdrant_client)` plus an explanatory
comment. `assert` is silently elided under `-O`; `cast` is the
conventional zero-cost narrower for branches the type checker can't
infer from the surrounding `if not initial_sync` ternary.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add chunk_start_offset / chunk_end_offset to _PAYLOAD_INDEX_FIELDS so
the legacy offset-based fallback in search/context.py works on Qdrant
Cloud strict mode (pre-#75 clients have no chunk_index payload).
- Cover chunk_index / chunk_start_offset / chunk_end_offset in the
payload-index summary test; refresh the stale field-list comment.
- Flag the is_valid_nextcloud_doc_id gate at both chunk-context handler
sites with a TODO for future non-numeric doc_types.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- _group_int_doc_ids: use type(value) is not int instead of isinstance,
since bool is an int subclass and would otherwise stringify to
"True"/"False" and corrupt legacy payloads on backfill.
- Replace doc_id.isdigit() guards in 5 boundary sites
(api/visualization, auth/viz_routes, search/context note/news_item/
deck_card branches) with a shared is_valid_nextcloud_doc_id helper
that rejects "0", leading zeros, and Unicode digit classes
(superscripts, Arabic-Indic, Devanagari) which pass isdigit() but
cannot be valid MySQL AUTO_INCREMENT IDs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add an .isdigit() guard at the top of both chunk-context handlers so a
non-numeric doc_id fails fast with a clear 400 ("doc_id must be numeric,
got 'abc'") rather than silently bottoming out as a 404 from deep inside
get_chunk_with_context. The earlier int(doc_id) coercion was removed when
doc_id became a pure pass-through to Qdrant's keyword payload index, which
also dropped this boundary validation.
Also align test_backfill_emits_progress_log_every_20_batches' scroll stub
with real Qdrant: next_offset is now "next-1" (str) instead of 1 (int),
matching the sibling test_backfill_rewrites_int_doc_ids_to_str. Pure
stub-fidelity fix; production code already treats next_offset as opaque.
Addresses both 🟡 Important items from PR #773 review round 10.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves both 🟡 important issues from the latest review:
1. `page_number` was unconditionally overwritten in `viz_routes.py:696` even
when Qdrant's payload lacked the field, clobbering the value resolved
from `chunk_context.page_number`. The new helper returns each field
independently and both call sites only overwrite via `is not None`
guards, matching the existing logic in `visualization.py`.
2. The ~60-line `if chunk_index is not None: ... else: ...` Qdrant scroll
block was duplicated between `api/visualization.py` and
`auth/viz_routes.py`. Extracted into `get_chunk_bbox_and_page_from_qdrant`
in `search/context.py` alongside the existing private `_get_chunk_*_from_qdrant`
helpers; both routes now share ~12 lines of caller code.
New unit tests at `tests/unit/test_chunk_bbox_helper.py` cover the indexed
and offset paths, the `(bbox, None)` regression case, and graceful
degradation on Qdrant strict-mode 400 (which also closes nit #4).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- search/context.py: rename triple-negation gate condition to
`skip_offset_lookup` named boolean for readability; convert new
logger.warning to lazy %-style per repo convention.
- api/visualization.py, auth/viz_routes.py: add comment on the offset-only
Qdrant scroll branch noting it is a legacy path for pre-astrolabe#75
clients and degrades gracefully on Qdrant Cloud strict mode.
Reviewer item #2 (extracting the duplicated scroll block into a shared
helper) deferred to a follow-up issue per the reviewer's "not blocking"
framing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Latest reviewer comment flagged five items on top of the original PR. This
commit addresses every one:
🟡 1. Skip the offset-based Qdrant fallback for `doc_type=file` when
`chunk_index` is supplied. Qdrant Cloud's strict mode rejects unindexed
filter fields with HTTP 400, which `_get_chunk_from_qdrant` catches and
logs at `logger.error` — masking real Qdrant problems in monitoring.
Notes/cards keep the offset fallback (cheap, useful for legacy data).
🟡 2. Add a `logger.warning` and clarifying inline comment in the doc-text
fallback path when `chunk_index` is None — surfaces the pre-existing
"0/N misreport" so callers can detect it. Type-nullability propagation
is deferred to a follow-up (out of scope for this hotfix).
🟢 3. Simplify `if chunk_text and doc_id_int is not None:` →
`if chunk_text:` with an inner `assert doc_id_int is not None` for
`ty` narrowing. The outer second clause was dead.
🟢 4. Add `doc_type` `FieldCondition` to the offset-based image lookup in
both `visualization.py` and `viz_routes.py` for parity with the
`chunk_index` branches.
🟢 5. Inline the `chunk_filter` local in `visualization.py` directly into
the `must=[]` list (matches `viz_routes.py` style).
Adds `tests/unit/test_chunk_context_offset_gate.py` with three regression
tests covering the gate matrix: (file, with-index → skip offset),
(note, with-index → still tries offset), (file, no-index → still tries
offset). Lives at top-level rather than `tests/unit/search/` to side-step
a pre-existing circular-init issue in `nextcloud_mcp_server.search`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-chunk PDF page renders (~150–700 KB base64 PNG each) were the dominant
disk consumer in production, repeatedly tripping `No space left on device:
WAL buffer size exceeds available disk space` on welcomed-malamute Qdrant.
Replace the inline highlighted_page_image / highlighted_page_number /
highlight_count fields with a small `chunk_bbox` field:
list[(x0, y0, x1, y1)] of normalized [0, 1] floats, ~32 bytes per chunk.
Astrolabe (the only known consumer) renders the highlight client-side as
a percentage-positioned overlay on top of the existing /api/v1/pdf-preview
render-on-demand path (cbcoutinho/astrolabe#76).
- pdf_highlighter: new compute_chunk_bboxes_batch() that reuses the
existing _find_chunk_bbox text-search path, skipping all pixmap/PIL/PNG
work.
- processor: store chunk_bbox + chunk_bbox_page in the Qdrant payload,
drop highlighted_page_image + friends, drop the base64 import.
- visualization /api/v1/chunk-context and auth/viz_routes: read
chunk_bbox instead of highlighted_page_image.
- vector/__init__: stop eagerly re-exporting `processor`/`scanner` —
fixes a pre-existing circular import (search.algorithms ->
vector.placeholder -> vector/__init__ -> processor -> scanner ->
server.semantic -> search.bm25_hybrid -> search.algorithms partial).
Test suite that was broken on master (test_bm25_hybrid.py et al.) now
collects and passes.
- scripts/purge_page_images.py: ad-hoc, idempotent migration that
delete_payload's the legacy keys from existing points. No reindex
required; legacy chunks render the page with no overlay.
Pairs with cbcoutinho/astrolabe#76. Frontend handles missing chunk_bbox
gracefully, so this can land in either order.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add `doc_type` FieldCondition to the chunk_index-path highlighted-image
Qdrant filter in both `api/visualization.py` and `auth/viz_routes.py`,
matching the shape of `_get_chunk_by_index_from_qdrant`. Safe today (the
block is guarded by `doc_type == "file"` and Nextcloud file IDs are
globally unique) but prevents a latent bug if other doc types start
storing highlighted images.
- Demote `viz_routes.py` `ValueError` log from `error` to `warning` (lazy
%-style) — `_parse_int_param` raises on user-supplied bad input, which
is a 400 not a server error and shouldn't pollute error logs.
- Hoist `effective_chunk_index` to compute once at the top of
`get_chunk_with_context`, removing two duplicate assignments.
- Add `test_file_doc_type_qdrant_miss_yields_fast_404` to the management
endpoint tests, locking in the proxy-timeout fix contract.
- Add `tests/unit/test_viz_routes_chunk_context.py` mirroring management
coverage for the OAuth-session route: param forwarding (chunk_index /
total_chunks), `doc_type=file` fast 404, and 400 on invalid int params.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Production was logging two cascading classes of Qdrant errors against the
welcomed-malamute deployment:
1. HTTP 400 — "Bad request: Index required but not found for \"doc_id\" of
one of the following types: [keyword]". The collection was created via
create_collection() with no payload indexes, so any FieldCondition
filter on doc_id failed at the Qdrant layer (placeholder writes/reads,
eviction, search context lookups).
2. Compounding the missing index, producers wrote a mix of int and str
doc_ids: webhook_parser stringified node_id, scanner stringified note
IDs, news IDs, and deck card IDs — but the file scanner passed the
numeric file_id through unchanged. A keyword index would not have
covered both kinds even if it had existed.
This change:
- Normalizes doc_id to str at every producer site (scanner.py:459,
DocumentTask.doc_id, indexed_*_ids reads from Qdrant).
- Tightens str|int annotations to str across placeholder.py,
eviction.py, search/verification.py, search/context.py,
SearchResult.id, and the auth/api visualization endpoints.
- Defensive str() coercion on doc_id reads in semantic.py /
bm25_hybrid.py / vector/visualization.py for the transition window
before the backfill runs.
- Adds an idempotent startup migration in get_qdrant_client():
- _ensure_keyword_payload_indexes creates KEYWORD indexes for
doc_id, user_id, and doc_type (tolerates "already exists" 400s).
- _backfill_doc_id_to_string scrolls the collection once and rewrites
int doc_ids to str. Skipped after a quick sample shows no legacy
int payloads.
- Public API preserved: SemanticSearchResult.id stays int via explicit
int(r.id) narrowing in server/semantic.py — surfaces a TypeError with
actionable context if a future doc_type ships non-numeric ids.
- Documents the startup migration in docs/configuration.md.
Tests: 11 new unit tests in tests/unit/vector/test_qdrant_client.py
covering happy path / already-exists / unrelated-400 for the index
helpers, and sample-skip / mixed-batch rewrite / payload=None edge cases
for the backfill. 889 unit tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related bugs surfaced in production while viewing chunks from the
Astrolabe frontend on the AWS-hosted MCP server:
1. PyMuPDF document closed: in _fetch_document_text the fallback path
referenced pdf_doc.page_count after pdf_doc.close(), raising
"document closed" and returning None. The slow PDF re-parse already
completed but its result was discarded. Capture page_count into a
local before close().
2. Slow/fragile chunk lookup: get_chunk_with_context filtered Qdrant by
(chunk_start_offset, chunk_end_offset). Those fields are not part of
the always-indexed payload schema, and with strict_mode enabled they
yield 400 errors. Even with manually-added indexes the filter is
fragile if a doc is re-chunked. Switch to chunk_index (always
indexed) as the primary lookup key, falling back to offset-based
lookup when callers don't supply it.
Plumb chunk_index/total_chunks through both the management API
(api/visualization.py) and the OAuth viz route (auth/viz_routes.py).
Apply the same change to the highlighted-image lookup so all four
chunk-context Qdrant queries prefer the indexed field.
Skip the slow PDF re-parse fallback entirely for files: when both the
chunk_index and offset Qdrant lookups miss, re-downloading and
re-parsing the source PDF won't find the chunk either, and routinely
exceeds 30s on large documents - which is the proxy timeout in
Astrolabe. Notes/cards keep the document-fetch fallback (cheap).
Removes dead code (_get_file_path_from_qdrant) that was only used by
the now-unreachable file fallback path.
Companion change in the Astrolabe app passes chunk_index from search
results through to the new endpoint params.
---
_This PR was generated with the help of AI, and reviewed by a Human_
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
428 (Precondition Required, RFC 6585) is the correct semantic — the
request requires the client to complete a prerequisite step (Login Flow
v2 provisioning) before retrying. 412 (Precondition Failed) is for
header-based preconditions like ETags / If-Match.
No behavior change beyond the status code; same JSON payload.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The webhook API endpoints in api/webhooks.py forwarded the inbound MCP
OAuth bearer token directly to Nextcloud as the Authorization header.
Per ADR-022 / docs/login-flow-v2.md the data leg from MCP server to
Nextcloud must use HTTP Basic Auth with the user's stored Login Flow v2
app password — bearer-forwarding requires upstream user_oidc patches that
were never merged and is incompatible with admin endpoints gated by
@PasswordConfirmationRequired (e.g. webhook_listeners/api/v1/webhooks,
which 401s).
PR #760 papered over the symptom for /api/v1/apps by switching to the
permissive /cloud/capabilities endpoint, but the same architectural
mistake remained on list_webhooks / create_webhook / delete_webhook,
which still 500'd on the astrolabe admin UI's preset page.
Changes:
- New helper api/_auth.py:get_basic_auth_for_user(user_id) reads the
user's app password from encrypted storage and returns
(username, app_password). Mirrors context.py:_get_client_from_login_flow
but is callable from Starlette routes (no MCP Context required).
- All four endpoints in api/webhooks.py now use httpx.BasicAuth instead
of forwarding the OAuth bearer; ProvisioningRequiredError is mapped to
HTTP 412 so callers can render a "complete provisioning" CTA rather
than receiving an opaque 500.
- Outbound NC requests now identify the user by the username recorded at
Login Flow v2 provisioning time (which may differ from the IdP-issued
user_id) — flowed into WebhooksClient and used for logging.
Tests:
- tests/unit/test_management_apps_endpoint.py: assertions updated to
verify outbound NC request uses BasicAuth and carries no Authorization
header. Replaced "missing-Authorization → 500" test with a
ProvisioningRequiredError → 412 case.
- tests/unit/test_webhooks_api_auth.py (new): cross-endpoint coverage
for list_webhooks, create_webhook, delete_webhook and the new helper —
including 412 symmetry for all four endpoints.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Astrolabe webhooks UI hits /api/v1/apps on the MCP server, which
forwarded the OAuth bearer token to /ocs/v1.php/cloud/apps?filter=enabled.
That OCS endpoint is admin-only AND @PasswordConfirmationRequired —
neither requirement is satisfiable via an OAuth bearer token, so even an
admin user's token returns a silent 401 (no entry in nextcloud.log).
Switch to /ocs/v2.php/cloud/capabilities, which has no admin or password-
confirmation gate, accepts the existing bearer token, and returns a
capabilities map keyed by app id (notes, files, tables, forms, etc.).
This is sufficient for the webhook presets UI to gate available presets
against the running Nextcloud instance's enabled apps.
Bearer is preserved on the outbound call because anonymous capabilities
omits notes/tables/forms — only authenticated capabilities exposes them.
Tests:
- New unit test covers the regression (asserts /ocs/v2.php/cloud/capabilities
is hit, NOT /cloud/apps), response parsing, sanitized error messages,
and missing-config paths.
- New integration test under tests/server/login_flow/ drives a real
OAuth flow against mcp-login-flow with a static OIDC client
(nextcloudMcpServerUIPublicClient) and asserts /api/v1/apps returns 200
with core/files in the response.
docker-compose.yml: aligns mcp-login-flow's ALLOWED_MGMT_CLIENT with
mcp-multi-user-basic so the same static-client test fixture works for both.
Follow-up to homelab-argocd #1608, which set ALLOWED_MGMT_CLIENT in
production but didn't unblock the webhooks flow.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The elicitation flow points users to the Astrolabe web route or the
BasicAuth REST endpoint to provision their app password. Both paths
stored the password without clearing the in-process scope cache, so a
user who provisioned through them would keep hitting
ProvisioningRequiredError for up to _SCOPE_CACHE_TTL (5 min) afterwards.
Add invalidate_scope_cache(user_id) to both write-paths (matching the
existing pattern in nc_auth_check_status), correct the now-misleading
comment in scope_authorization.py to name all three invalidation paths,
and add a one-line hint above the first elicitation patch in the test
file so future authors don't "fix" the patch target to the wrong module.
Addresses PR #757 round-3 review feedback.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address round-5 reviewer feedback on PR #747:
- Escape `webhook_uri` in the admin pane HTML template so an operator-
controlled env value (`WEBHOOK_INTERNAL_URL`, `NEXTCLOUD_MCP_SERVER_URL`)
can't inject markup. The sibling `preset_id` and exception messages were
already escaped — this one was the odd one out.
- Convert the eight remaining f-string `logger.warning`/`logger.error`
calls in `api/webhooks.py` to lazy `%s` formatting, matching the style
already adopted by `webhook_receiver.py` and `webhook_routes.py`.
- Document why the 401 from `handle_nextcloud_webhook` deliberately omits
`WWW-Authenticate`: NC's webhook delivery worker has no auth-flow state
machine to negotiate against, the bearer is a static shared secret
configured out-of-band via `WEBHOOK_SECRET`, and a challenge response
wouldn't change client behaviour. The existing warning log already
records the rejection.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds optional shared-secret authentication for /webhooks/nextcloud,
addressing the security follow-up flagged in #747.
Behavior:
- WEBHOOK_SECRET set: registrations pass authMethod="header" with
authData={"Authorization": "Bearer <secret>"} (encrypted at-rest in
Nextcloud's DB and forwarded on every delivery). The receiver
validates the same header with hmac.compare_digest before parsing
any payload; missing/invalid → 401.
- WEBHOOK_SECRET unset: registrations stay on authMethod="none" and
the receiver accepts unauthenticated POSTs (logging a one-time
startup warning). Backward compatible — operators can roll out at
their own pace.
Implementation notes:
- WebhooksClient.create_webhook gains an `auth_data` parameter mapped
to NC's `authData` body field; this is distinct from the existing
`headers` parameter (`headers` is plaintext static request headers,
`authData` is encrypted at-rest in NC and only emitted when
authMethod="header"). The previous `auth_method="bearer"` mention in
the docstring was incorrect — NC supports only "none" and "header".
- A small `webhook_auth_pair()` helper in auth/webhook_routes.py
centralises the secret→(auth_method, auth_data) resolution so the
preset flow and the Astrolabe-facing /api/v1/webhooks endpoint stay
in sync.
Also addresses the smaller review points from #747:
- f-string → lazy %s formatting in webhook_receiver.py and
webhook_routes.py.
- Move `int(time)` inside webhook_parser's try/except so a malformed
`time` field returns None instead of raising ValueError.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The /api/v1/chunk-context and /api/v1/pdf-preview handlers in
api/visualization.py forwarded the incoming OAuth bearer directly to
Nextcloud via NextcloudClient.from_token. In multi-user BasicAuth mode
Nextcloud has no validator for those bearers on Notes/WebDAV, so it
treats the request as anonymous and returns 401 — surfaced to the user
as a 500 from /apps/astrolabe/api/chunk-context. Search worked because
it only hits Qdrant.
Architecturally, OAuth is only for Astrolabe→MCP server; MCP server→
Nextcloud always uses the per-user app password stored during provision
(background sync already does this via vector.oauth_sync).
- Resolve the Nextcloud client through get_user_client_basic_auth in
both get_chunk_context and get_pdf_preview, surfacing
NotProvisionedError as a clean 401 instead of opaque 500.
- Apply the same fix to the session-cookie variant in
auth/viz_routes.chunk_context_endpoint for the internal viz UI.
Tests:
- New unit file test_management_chunk_context_endpoint.py, including a
regression guard that asserts get_user_client_basic_auth is awaited
(so reverting to from_token fails without needing a live Nextcloud).
- Updated test_management_pdf_preview_endpoint.py to mock the new auth
path (drops extract_bearer_token / NextcloudClient.from_token patches).
- New integration test test_astrolabe_chunk_context.py drives the full
chain (browser → Astrolabe → MCP → Nextcloud) in multi-user BasicAuth
mode, plus bare-bones 401 checks on the MCP endpoint.
Full unit suite: 546 passed.
Companion PR on astrolabe (cbcoutinho/astrolabe#66) sends the Nextcloud
UID as loginName in the app-password POST body so the stored record is
complete. Submodule bump to that branch will follow once CI reproduces
the failure on the old submodule.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Smithery is no longer a supported deployment mode. Remove all Smithery-specific
code paths, middleware, configuration, and tests. This simplifies the codebase
by eliminating DeploymentMode enum, SmitheryConfigMiddleware, session config
context variables, and the smithery_main entrypoint.
Files deleted: Dockerfile.smithery, smithery.yaml, smithery_main.py
ADR-016 retained with deprecated status for historical reference.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add 429 retry with exponential backoff to register_client() (fixes CI
oauth matrix failures from parallel DCR requests)
- Make client_id, redirect_uri, and PKCE mandatory at token endpoint
- Add null-checks for discovery_url and OAuth credentials in proxy flows
- Add OIDC discovery document caching with 5-min TTL
- Add per-IP rate limiting on /oauth/register DCR proxy
- Discover DCR endpoint from OIDC discovery instead of hardcoding
- Extract extract_user_id_from_token to auth/token_utils.py (breaks
circular imports between server/ and auth/ layers)
- Add TTL scope cache in scope_authorization.py (avoids DB hit per tool)
- Add defense-in-depth scope validation in storage layer
- Broaden elicitation exception handling with graceful fallback
- Add idempotentHint to nc_auth_check_status, return "pending" status
after accepted elicitation, add polling interval to description
- Change ALL_SUPPORTED_SCOPES from tuple to frozenset for O(1) lookups
- Replace Optional[str] with str | None throughout config.py
- Use default_factory for ProxyCodeEntry/ASProxySession dataclasses
- Add proxy code/session cleanup to background loop
- Fix OIDC verification CI step to only run for oauth/login-flow modes
- Add unit tests for access.py REST endpoints (10 tests)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Consolidate MCP session + login flow cleanup into _mcp_session_with_login_flow() helper,
replacing 4 duplicated AsyncExitStack sites in app.py
- Fix get_shared_storage() race condition by using module-level anyio.Lock() init
(reverts regression from ba59763)
- Collapse cosmetic if/else branching in scope_authorization.py
- Consolidate dual password storage paths into single store_app_password_with_scopes() call
- Mark unused request param as _ in list_supported_scopes
- Make ALL_SUPPORTED_SCOPES an immutable tuple; use list() instead of .copy()
- Add hasattr(ctx, "elicit") guard in elicitation.py, narrow except to NotImplementedError
- Add YAML comment explaining --oauth flag for mcp-login-flow service
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Consolidate three independent RefreshTokenStorage lazy singletons into a
single lock-protected get_shared_storage() function, eliminating race
conditions on concurrent first-access. Remove blanket try/except in
_get_stored_scopes so storage errors propagate as proper MCP errors
instead of silently triggering "please provision" messages. Handle
declined/cancelled elicitation results in Login Flow tools by cleaning up
sessions and returning clear status. Add update_app_password_scopes() to
avoid unnecessary decrypt/re-encrypt when only scopes change. Add
unprovisioned-user early exit and no-op detection to nc_auth_update_scopes.
Remove four dead config fields and misleading NEXTCLOUD_PASSWORD deprecation
warning. Add periodic login flow session cleanup task. Generate separate
Fernet keys per service. Add board cleanup in deck integration test. Gate
CI unit tests on linting and skip Astrolabe build for single-user profile.
Fix test markers from oauth to multi_user_basic for astrolabe integration
tests. Update login_flow.py docstrings to document outbound HTTP calls.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Enable ruff PLC0415 rule for all source files (tests excluded via
per-file-ignores). Move 136 inline imports to top-level across 33 files.
8 imports suppressed with noqa for legitimate reasons: circular
dependencies (client/__init__.py, context.py), optional dependency
guards (app.py document processors, auth/userinfo_routes.py), and
post-env-setup imports (smithery_main.py).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add NEXTCLOUD_VERIFY_SSL and NEXTCLOUD_CA_BUNDLE env vars to configure
TLS certificate verification for all outbound Nextcloud connections.
Centralizes SSL config via a new HTTP client factory (http.py) used by
all 27 Nextcloud-bound call sites, including API clients, OIDC endpoints,
OAuth flows, and health checks.
Closes#560
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
