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>
Two findings from the reviewer's latest pass:
- config_validators.py:329: comment in the LOGIN_FLOW validation block
still referenced `_sync_derived_flags` (removed in commit 5; derivation
now lives in `Settings.__post_init__`). Updated the comment to point at
the correct location so a future reader grepping for the function name
doesn't come up empty.
- tests/unit/test_config_validators.py: added
`test_oauth_single_audience_migration_hint` next to the existing
`test_invalid_deployment_mode_raises_error`. The new test pins the
ADR-022 rename-hint branch in `detect_auth_mode` by setting
`MCP_DEPLOYMENT_MODE=oauth_single_audience` and asserting the
ValueError mentions both the old and new mode names plus "ADR-022".
Without this, a future refactor could drop the hint without any test
catching it (the prior `invalid_mode` test only asserts the generic
"Valid values:" prefix).
No functional changes; 1010 unit tests now pass (+1 from the new hint
test).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four small follow-ups from the reviewer's latest pass:
- tests/unit/test_stdio.py:18: the single_user_env fixture used
monkeypatch.delenv("ENABLE_MULTI_USER_BASIC_AUTH", ...). That env var
is no longer read after the ADR-022 follow-up; switched to delenv of
MCP_DEPLOYMENT_MODE which is the canonical mode-selection input today.
Comment updated to match.
- config_validators.py: when detect_auth_mode rejects an invalid
MCP_DEPLOYMENT_MODE, surface a one-line ADR-022 migration hint if the
rejected value is exactly "oauth_single_audience" (the most common
upgrade pain — users carrying that value over from ADR-021 .env files).
Other invalid values get the regular "Valid values: …" message
unchanged.
- config.py + config_validators.py: added cross-reference comments on
both mode-resolution sites (Settings.__post_init__ and
detect_auth_mode) noting that they each compute the canonical mode
independently and must be kept in sync when a new mode is added.
Surfaces the parallel-duplication intentionally so the next maintainer
doesn't have to discover it.
- docs/ADR-021-configuration-consolidation.md:92: appended a trailing
comment to the historical "valid values" example, marking
oauth_single_audience and oauth_token_exchange as removed in ADR-022.
ADR-021 stays as the historical record; the trailer points future
readers at the current state.
No functional changes; 1009 unit tests still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five small findings from the reviewer's third round, plus a SonarCloud
quality-gate failure on a test fixture.
- docs/troubleshooting.md, docs/configuration.md: six pre-PR references
to a non-existent `login_flow_v2` mode value (the actual enum value is
`login_flow`). They predated this PR but became actively misleading
once `detect_auth_mode` started raising ValueError for anything not in
the mode_map. Replaced with `login_flow` via sed.
- docs/configuration-migration-v2.md: removed a duplicate
`MCP_DEPLOYMENT_MODE=multi_user_basic` line in the troubleshooting
section (around line 447) — same shape as the round-2 duplicate
caught earlier in the migration-steps section. Also dropped the
`oauth_token_exchange` row from the mode-value table around line 364
(that enum value was removed in 57303135 and would now raise
ValueError from detect_auth_mode).
- nextcloud_mcp_server/config.py: field comments for
`enable_multi_user_basic_auth` and `enable_login_flow` said
"Auto-set by detect_auth_mode()" but the derivation moved into
`Settings.__post_init__` in the previous commit. Updated both.
- tests/unit/test_config_validators.py: SonarCloud's python:S2068
flagged `nextcloud_password="hunter2"` in the
`test_login_flow_mode_auto_derives_enable_login_flow_flag` fixture I
added in commit 5 as a potentially hard-coded credential. Other
fixtures in the same file use the literal `"password"` and aren't
flagged (they predate the PR and SonarCloud only checks new-code).
Switched to `"password"` to match the existing convention.
No functional changes; all 1009 unit tests still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The integration jobs for `mcp-multi-user-basic` and `mcp-login-flow`
were failing with HTTP 500s. Root cause: `get_settings()` builds a
fresh Settings on every call (not cached). Commits 3 and 4 set the
derived `enable_login_flow` / `enable_multi_user_basic_auth` flags as
a side effect of `detect_auth_mode`. detect_auth_mode runs once at
startup, against the Settings instance owned by `validate_configuration`.
Every per-request call site that does `settings = get_settings()` got
a fresh Settings with both flags at their default `False` (since the
env-var aliases were dropped), causing the multi-user dispatcher in
`context.py` to take the wrong branch and crash.
Fix: move the derivation into `Settings.__post_init__`. Every Settings
instance now carries correct flags from the moment it's constructed —
no caching needed, no mutation-after-construction race. detect_auth_mode
becomes a pure reader of the already-derived state.
The legacy env-var deprecation check moves with it. It also picks up
the reviewer's truthy-string fix: previously `os.getenv(legacy)` fired
for the literal string "false" (a non-empty Python string is truthy),
which would have errored on any user with a leftover
`ENABLE_LOGIN_FLOW=false` in their `.env`. The check now only fires
when the value lowercases to one of {"1", "true", "yes", "on"}.
- nextcloud_mcp_server/config.py: extend Settings.__post_init__ with
the legacy-deprecation block and the derived-flag derivation
(resolve mode from deployment_mode + username/password, set flags).
- nextcloud_mcp_server/config_validators.py: drop the
`_sync_derived_flags` helper (superseded by __post_init__). Drop the
legacy-env-var deprecation block (moved). `detect_auth_mode` is now
pure — no mutation. Drop the now-unused `import os`.
- tests/unit/test_config_validators.py: legacy-env-var tests now
expect `ValueError` at `Settings(...)` construction (via `get_settings()`),
not at `detect_auth_mode` call. Added two new tests:
* `test_legacy_env_var_check_ignores_falsy_strings` — pins the
truthy-string fix (reviewer round 2 finding).
* `test_derived_flags_stable_across_get_settings_calls` — regression
test pinning the integration-test fix (two consecutive
`get_settings()` calls return Settings instances with the same
derived flags).
Also reworked `test_login_flow_mode_auto_derives_enable_login_flow_flag`
to assert at-construction derivation (not the old mutation pattern).
- docs/configuration-migration-v2.md: dropped the duplicate
`MCP_DEPLOYMENT_MODE=multi_user_basic` line (review round 2 nit — a
sed artifact from commit 4).
- docs/ADR-021-configuration-consolidation.md: sed-replaced the in-body
`MCP_DEPLOYMENT_MODE=oauth_single_audience` examples with `login_flow`
(review round 2 nit — only the status header was updated in commit 4).
- tests/conftest.py: docstring comment for the multi-user-basic fixture
switched from `ENABLE_MULTI_USER_BASIC_AUTH=true` to
`MCP_DEPLOYMENT_MODE=multi_user_basic` (review round 2 nit).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
Once OAUTH_SINGLE_AUDIENCE was renamed to LOGIN_FLOW and the validation
gate ensured the only meaningful configuration was
`MCP_DEPLOYMENT_MODE=login_flow + ENABLE_LOGIN_FLOW=true`, the two
controls became redundant. Setting the mode is sufficient; the
ENABLE_LOGIN_FLOW env var doesn't add information.
This commit makes the deployment mode the single source of truth for
the Login Flow v2 toggle:
- `nextcloud_mcp_server/config.py`: drop the `ENABLE_LOGIN_FLOW`
dynaconf env-var alias. The `enable_login_flow` field stays as an
internal attribute so the 6 runtime call sites (app.py x4,
context.py, auth/scope_authorization.py) keep working unchanged.
Updated field docstring to flag it as derived.
- `nextcloud_mcp_server/config_validators.py`:
- Drop `enable_login_flow` from `MODE_REQUIREMENTS[LOGIN_FLOW].required`.
- Drop the validation gate that required ENABLE_LOGIN_FLOW=true for
LOGIN_FLOW mode (no longer possible to misconfigure — the flag is
derived, not user input).
- Add `_sync_derived_flags()` helper called at every return path of
`detect_auth_mode` to set `settings.enable_login_flow` from the
resolved mode.
- `tests/unit/test_config_validators.py`: drop `enable_login_flow=True`
from happy-path fixtures (no longer needed — detection sets it).
Repurpose `test_login_flow_requires_enable_login_flow_flag` into
`test_login_flow_mode_auto_derives_enable_login_flow_flag` which
asserts the new auto-derivation behaviour for both LOGIN_FLOW and a
non-LOGIN_FLOW mode.
- `docker-compose.yml`: remove `ENABLE_LOGIN_FLOW=true` from the
`mcp-login-flow` and `mcp-keycloak` profiles.
- `env.sample`: remove the ENABLE_LOGIN_FLOW reference; the comment
on `MCP_DEPLOYMENT_MODE` now notes the derived flag.
- `docs/configuration.md`, `docs/authentication.md`,
`docs/login-flow-v2.md`, `docs/auth-flows.md`,
`docs/troubleshooting.md`, `docs/ADR-025-*.md`: replace
ENABLE_LOGIN_FLOW=true examples and references with
MCP_DEPLOYMENT_MODE=login_flow.
BREAKING CHANGE: `ENABLE_LOGIN_FLOW` is no longer read from the
environment. Anyone who relied on `ENABLE_LOGIN_FLOW=true` to activate
Login Flow v2 should set `MCP_DEPLOYMENT_MODE=login_flow` instead (or
rely on it being the default when no other auth env vars are set).
Co-Authored-By: Claude Opus 4.7 (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>
The CalDAV REPORT in `_search_events_by_date` unconditionally requested
server-side `<C:expand>`. Per RFC 4791 §9.6.5 the server then normalizes
every expanded DTSTART/DTEND to UTC `Z`, which destroyed two pieces of
information on the read path:
- RFC 5545 floating local times came back as fake-UTC (a `+00:00` suffix
that did not match the stored value), so a 2:30 PM floating event was
indistinguishable from a 14:30 UTC event in the MCP response.
- TZID-bound events lost their IANA TZID context — a "10am America/New_York"
event came back as `14:00:00+00:00`, making it impossible for callers to
reconstruct DST-aware recurrence semantics.
Replace `<C:expand>` with client-side recurrence expansion via the
`recurring-ical-events` library (promoted from transitive to direct dep),
so the wire response retains its original DTSTART format. Surface the
TZID parameter as new `start_tz`/`end_tz` fields on `CalendarEventSummary`.
Add an optional `timezone` (IANA name) parameter to `nc_calendar_create_event`
and `nc_calendar_update_event` so callers can pin a TZID for naive input;
the helper attaches `ZoneInfo(...)` and emits a paired `VTIMEZONE`
component. Naive input without `timezone` continues to store as RFC 5545
floating local time (with a warning logged). Offset-aware input continues
to store as UTC `Z`.
Drive-by: switch the update path's DTSTART/DTEND assignment from raw
`datetime` to `vDDDTypes(dt)` wrappers — the previous code produced invalid
iCal like `DTSTART:2026-05-14 10:00:00+00:00` for any TZ-aware update.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #781 review round 1:
- 🔴 Fix notesPath key: the Notes API returns the folder under camelCase
``notesPath`` (see models/notes.py:43), but `deck_attach_note` was
looking up snake_case ``notes_path`` and silently falling back to
``"Notes"``. Users with a non-default notes folder would have produced
shares pointing at non-existent files (404 on click in Deck UI).
- 🔴 Add wire-through unit test that would have caught the above:
extract `_resolve_note_attach_path(client, note_id)` as a testable
helper that encapsulates the camelCase-key lookup. Three new tests:
custom notesPath honored, missing key falls back to default, null
category handled.
- 🟡 Modernize new fields on `DeckAttachmentExtendedData` to PEP 604
(`X | None`) per CLAUDE.md.
- 🟡 Drop unnecessary string forward reference on
`ListAttachmentsResponse.results` — DeckAttachment is defined earlier
in the same module.
- 🟢 Move `pytestmark = pytest.mark.unit` to module level in
test_sharing_client.py to match the convention in test_deck_server.py.
Per user request: `deck_attach_file` is now scoped `deck.write` +
``files.read`` (was just `deck.write`) so the generic file-share
permission story is consistent — only `deck_attach_note` keeps
`notes.read` since it specifically reads from the Notes app. Docstring
updated to emphasise the tool is generic over the user's Files
(PDFs/images/etc., not just markdown).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds four tools that expose Deck card attachments via the MCP surface:
deck_attach_file, deck_attach_note, deck_list_attachments, and
deck_delete_attachment. The attach* variants share an existing Files
entry (or Notes-app note) with the card via OCS shareType=12 — same
mechanism the Deck UI's "Share from Files" picker uses, no file copy.
This replaces the prior workaround of appending bulky activity content
as Deck card comments: per-PR/per-event narrative now lives in NC Notes
and surfaces on the tracking card as a clickable attachment that opens
the original note in place.
Implementation reuses existing client methods (SharingClient.create_share,
DeckClient.get/delete_attachment, NotesClient.get_settings/get_note);
no new client code. _SHARE_TYPE_DECK is centralised with a CI-guard test
to prevent silent drift, and SharingClient.create_share's wire format is
pinned to what the Deck Vue source sends.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 17 reviewer (🟡 Important):
1. docs/configuration.md degraded-migration runbook said `doc_id backfill
failed on …` but the actual log line in qdrant_client.py:415 is
`doc_id backfill scroll failed on …`. Operators grepping the runbook
string would have missed it. Insert the `scroll` qualifier.
2. _create_one_payload_index returned True on the 400 schema-conflict
path, so a wrong-type index discovered at create time skipped the
consolidated `Payload index creation incomplete` summary — but a
wrong-type index discovered via the existing-schema check at line
195-206 did fire it. Tenants whose payload_schema is hidden from
their JWT (Qdrant Cloud collection-scoped tokens) only ever observe
the create-time path, so they never saw the operator-level summary.
Return False so the summary fires in both cases.
3. docs/configuration.md said the upgrade-time delay was `proportional to
point count while writes are issued` — overstating the cost. Writes
are proportional to int-typed points only; the scroll itself is
proportional to total point count. Reword.
Local-mode collection-creation regression (root-cause of failing
single-user / login-flow / multi-user-basic CI jobs):
PR #779 changed the existence probe in get_qdrant_client from
collection_exists() (returned bool in both modes) to get_collection()
+ except UnexpectedResponse(status_code=404). The HTTP-mode client
raises UnexpectedResponse with a 404 body, but the local/in-memory
client raises ValueError(f"Collection {name} not found") — see
qdrant_client/local/async_qdrant_local.py. The narrow except clause
let the ValueError propagate, app.py's lifespan re-raised as
RuntimeError, and the mcp container crashed on first start. Catch
ValueError too, with a `not found` substring guard so genuine
programming bugs (bad collection_name, etc.) still surface.
Tests: extend the existing 400-path test to assert the new
failed_fields contract; add two get_qdrant_client unit tests pinning
the local-mode VE catch (positive case + propagation case).
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>
Defer publication of `_qdrant_client` until after the in-lock backfill +
payload-index migration awaits complete. The fast-path check at the top of
`get_qdrant_client` reads the singleton without holding the init lock, so
publishing the constructed-but-unmigrated client let concurrent fast-path
callers fire filtered searches before `_ensure_payload_indexes` ran —
producing HTTP 400 ("Index required but not found") on Qdrant Cloud strict
mode. Local `provisional` is now used for every await inside the lock; the
global is assigned exactly once, last.
Replace the five hand-rolled `scroll(..., limit=10000)` calls in
`vector/scanner.py` (notes / files / news / deck-cards deletion tracking,
plus the timestamp scroll) with a single paginated `_scroll_all_points`
helper. The previous single-page cap silently dropped deletion-tracking
points beyond the first 10 k for any user past that threshold. Pagination
follows Qdrant's documented contract (loop until `next_page_offset is
None`) with a fixed per-page `_DELETION_TRACKING_PAGE_SIZE = 1024`.
Extract `_create_one_payload_index` from `_ensure_payload_indexes` to drop
its cognitive complexity below the SonarQube limit (17 → ≤ 15) without
losing the per-field error-containment rationale; every comment is
preserved verbatim on the helper.
Drop the stale `SearchResult.id` `int | str` comment and the redundant
`str(d)` coercion in `_verify_news_items` — the contract has been
str-only since the producer-side stringification landed earlier in this
PR.
Fix eight `doc_id=<int>` test calls in `test_chunk_context_offset_gate.py`
that violated the `doc_id: str` signature of `get_chunk_with_context`,
plus align `_make_result` in `test_verification.py` to coerce `id=str(...)`
matching the production contract — and update 30+ assertions from int
sets (`{1, 2, 3}`) to str sets (`{"1", "2", "3"}`) so the tests now model
the post-PR `SearchResult.id: str` reality end-to-end. Previously these
were masked by the `str(d)` coercion now removed from production.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add chunk_start_offset / chunk_end_offset to _PAYLOAD_INDEX_FIELDS so
the legacy offset-based fallback in search/context.py works on Qdrant
Cloud strict mode (pre-#75 clients have no chunk_index payload).
- Cover chunk_index / chunk_start_offset / chunk_end_offset in the
payload-index summary test; refresh the stale field-list comment.
- Flag the is_valid_nextcloud_doc_id gate at both chunk-context handler
sites with a TODO for future non-numeric doc_types.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- _group_int_doc_ids: use type(value) is not int instead of isinstance,
since bool is an int subclass and would otherwise stringify to
"True"/"False" and corrupt legacy payloads on backfill.
- Replace doc_id.isdigit() guards in 5 boundary sites
(api/visualization, auth/viz_routes, search/context note/news_item/
deck_card branches) with a shared is_valid_nextcloud_doc_id helper
that rejects "0", leading zeros, and Unicode digit classes
(superscripts, Arabic-Indic, Devanagari) which pass isdigit() but
cannot be valid MySQL AUTO_INCREMENT IDs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- search/context.py: drop the doc_type=='file' guard on skip_offset_lookup
so notes / deck cards / news items also bypass the unindexed offset
fallback when chunk_index is available. Legacy chunk_index=None data
still uses the offset path.
- vector/qdrant_client.py: clarify the backfill/_ensure_payload_indexes
ordering invariant (backfill rewrites payload values only, never schema
or indexes). Acknowledge OSS-vs-Cloud uncertainty in the 400-branch
comment and the new-collection call-site comment.
- vector/scanner.py: hoist qdrant_client to function scope so the
file-scroll block doesn't depend on a name bound inside the
notes-scroll block.
- tests/unit/test_chunk_context_offset_gate.py: flip the note-with-
chunk_index test to assert the offset fallback is skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three coordinated fixes flagged as Important in the round-10 review of
PR #773:
1. Index chunk_index. The chunk-context fast path in
_get_chunk_by_index_from_qdrant and get_chunk_bbox_and_page_from_qdrant
filters on chunk_index, but the field was absent from
_PAYLOAD_INDEX_FIELDS. On Qdrant Cloud strict mode every chunk-context
lookup via chunk_index would 400 and silently fall back to the
document re-fetch path — the exact failure mode the chunk_index
shortcut exists to avoid. Added as INTEGER schema.
2. Catch raw network errors in _ensure_payload_indexes. The
create_payload_index loop only caught UnexpectedResponse, so an
httpx.ConnectError or asyncio.TimeoutError mid-loop would propagate
uncaught — leaving _qdrant_client assigned and silently skipping all
remaining fields. Added a broad Exception catch with the same
per-field containment as the 5xx path: log at ERROR with exc_info,
append to failed_fields, continue. New test covers the path.
3. Lazy-initialise _qdrant_init_lock. Constructing anyio.Lock() at
module import time works for the asyncio backend but anyio's docs
advise instantiating synchronization primitives within an async
context, and pyproject.toml's anyio_mode = "auto" means tests can
run under trio. Moved the construction into get_qdrant_client; safe
under cooperative multitasking because there is no await between the
None-check and the assignment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add an .isdigit() guard at the top of both chunk-context handlers so a
non-numeric doc_id fails fast with a clear 400 ("doc_id must be numeric,
got 'abc'") rather than silently bottoming out as a 404 from deep inside
get_chunk_with_context. The earlier int(doc_id) coercion was removed when
doc_id became a pure pass-through to Qdrant's keyword payload index, which
also dropped this boundary validation.
Also align test_backfill_emits_progress_log_every_20_batches' scroll stub
with real Qdrant: next_offset is now "next-1" (str) instead of 1 (int),
matching the sibling test_backfill_rewrites_int_doc_ids_to_str. Pure
stub-fidelity fix; production code already treats next_offset as opaque.
Addresses both 🟡 Important items from PR #773 review round 10.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the four 🟡 important findings from claude-bot review on PR #773:
str (non-Optional) and the guard would silently skip the Qdrant lookup
for an empty string. Removing the guard matches the type signature.
(`all([…, doc_id, …])` rejects None and empty string, plus
`assert doc_id is not None`). No code change needed.
`get_qdrant_client()` with a module-level `anyio.Lock`. Double-checked
locking keeps the steady-state hot path lock-free. Without this,
parallel cold-start callers could all enter the init block and run
`_backfill_doc_id_to_string` + `_ensure_payload_indexes` redundantly
(idempotent, but noisy). Pattern matches `auth/storage.py:2071`.
behavior with three tests covering the float-warning path (the gap
called out in the review), the str/None silent-skip paths, and the
int-grouping happy path.
Verification:
- ruff check / format: clean
- ty check -- nextcloud_mcp_server: clean
- uv run pytest tests/unit/: 969 passed
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two unrelated CI failures on this branch, one fix each:
- tests/integration/test_deck_vector_search.py: pass str(card.id) to
get_chunk_with_context. The function's contract is doc_id: str
(keyword-indexed in Qdrant), and real callers (viz_routes.py URL
path, server/semantic.py via str(result.id)) all stringify. The
test was the only int caller, hitting the .isdigit() guard added
earlier on this branch.
- tests/server/login_flow/test_login_flow_integration.py:
test_check_status_provisioned now accepts scopes=None as valid.
Per ProvisionStatusResponse in models/auth.py, None is the
documented sentinel for "all scopes granted" — and the web
provisioning path (provision_routes.py, used by Astrolabe's
"Enable Semantic Search" flow exercised by the new regression test
added on this branch) stores exactly that. The previous
is-not-None assertion hid behind test order until that flow ran.
- Replace anyio.sleep(0) with anyio.lowlevel.checkpoint()
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Skip and warn instead of stringifying floats / unexpected types in the
backfill helper. A stray doc_id=3.0 would otherwise be rewritten to
"3.0", which producers (str(int)) and the keyword index would never
match, and which int() on the verification side would reject. Also add
a doc_id=0 case to the backfill test to guard against a future
falsy-skip regression.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add the missing end-to-end coverage for the seam that PR #773's
`ALLOWED_MGMT_CLIENT` ↔ `astrolabeMcpClientOAuth00000000000` drift
bug exposed: browser → Astrolabe (NC PHP app) → MCP server's
management API on the `mcp-login-flow` profile. Every existing
`tests/integration/test_astrolabe_*.py` is marked `multi_user_basic`
and exercises the BasicAuth flow, not Login Flow v2 / OAuth.
The new test mirrors the production-shaped flow exactly:
1. Log in as admin via Playwright.
2. Navigate to `/settings/user/astrolabe`.
3. Click the "Enable Semantic Search" OAuth link rendered by
`oauth-required.php`. (Same selector Astrolabe's own e2e helper
uses — `third_party/astrolabe/tests/e2e/helpers/authorize.ts`.)
4. Click "Allow" on the Nextcloud OIDC consent screen.
5. Wait for the redirect back to the Astrolabe settings page.
6. Assert the "Enable Semantic Search" link is no longer visible.
Step 6 is the canary for the drift class: if Astrolabe's management
API call to `/api/v1/users/{id}/session` is rejected (HTTP 401, the
original bug), the session lookup falls back to "no token" and the
same `oauth-required.php` template re-renders with the link still
present — so the test fails loudly with a message naming the
likely cause.
Reuses `login_to_nextcloud` and `navigate_to_astrolabe_settings`
helpers from `tests/integration/test_astrolabe_multi_user_background_sync.py`
(already pattern-imported by the Plotly viz test). No fixture-level
OIDC client creation: `app-hooks/before-starting/26-configure-astrolabe-oauth.sh`
already provisions `astrolabeMcpClientOAuth00000000000` with the
correct redirect URI and scopes when `MCP_SERVER_URL` is set in the
shell that runs `docker compose --profile login-flow up`.
The test skips cleanly when admin is already authorized (typical
state on a re-run against a long-lived dev stack), so it's safe to
run repeatedly without manual reset.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🔴 Blocking finding from PR #773 latest review:
`get_chunk_bbox_and_page_from_qdrant` (`search/context.py:199`) still
declared `doc_id: int | str` and passed the raw value into
`MatchValue(value=doc_id)` at lines 239 and 256 without `str()`
coercion. After this branch's startup backfill normalises every Qdrant
`doc_id` payload to a string, an `int` filter would silently match zero
points — the function would return `(None, None)` instead of the
chunk bbox / page, and PDF highlight overlays would fail in production.
Take option 2 from the reviewer's two suggestions (annotation
tightening over inline coercion): the producer side of this PR has
already narrowed every other `doc_id` annotation to `str`, so this
function is the last hold-out. Pushing the contract into the type
system means `ty` will catch any future regression at the call site.
Production callers in `api/visualization.py` and `auth/viz_routes.py`
already pass `doc_id` (str) verbatim after the recent merge with
master's chunk_index-first refactor, so no caller-side changes needed.
Update the 9 calls in `tests/unit/test_chunk_bbox_helper.py` to use
string literals (`"42"` / `"99"` / `"1"`) instead of integers. The
mock doesn't validate `MatchValue` value types, so the tests passed
with stale int doc_ids today — but they were exercising a path
production no longer takes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit moved `mcp-login-flow`'s `ALLOWED_MGMT_CLIENT` to
`astrolabeMcpClientOAuth00000000000` so production-shaped Astrolabe
traffic actually validates. Update the management API test fixture to
match: the static OIDC client created in
`tests/server/login_flow/conftest.py:login_flow_static_client_credentials`
now uses the same id `app-hooks/before-starting/26-configure-astrolabe-oauth.sh`
provisions in real deployments, so the test path exercises the same
code as production rather than a substituted fixture-only id.
`mcp-multi-user-basic`'s allowlist is unchanged
(`nextcloudMcpServerUIPublicClient`) and the shared
`configure_astrolabe_for_mcp_server` fixture in `tests/conftest.py`
keeps that as its default, so multi-user-basic tests are unaffected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer findings (1 blocking + 2 important):
- 🔴 Replace `import asyncio` / `await asyncio.sleep(0)` with
`import anyio` / `await anyio.sleep(0)` in the four async side-effect
helpers (_scroll_raises, _upsert_raises, _get_collection_raises,
_create_index). CLAUDE.md mandates anyio for all async operations;
conftest pins the backend to asyncio so the asyncio.sleep call worked
today, but the inconsistency would surface the moment that pin moves.
- 🟡 Replace the sentinel's zero dense vector with a single non-zero
element (`[1e-9] + [0.0] * (dimension - 1)`). Cosine distance is
mathematically undefined for the zero vector and Qdrant Cloud strict
mode rejects zero-vector upserts. The exact value doesn't matter
(sentinel never participates in a search — no user_id/doc_id/doc_type
payload) but the upsert itself must be valid.
- 🟡 Avoid the duplicate `get_collection` round-trip on every restart.
`_ensure_payload_indexes` now accepts an optional
`existing_schema: dict | None` parameter; when None it fetches
collection_info itself (and the get_collection-failure swallow still
applies), but `get_qdrant_client` already fetches collection_info
for dimension validation in the existing-collection branch — pass
`collection_info.payload_schema or {}` through to skip the second
call. The new-collection branch passes `existing_schema={}`
explicitly since a freshly created collection has no payload schema.
The 🟡 deck_card iteration-fallback finding doesn't apply: the
`isdigit()` guard at context.py:612 returns early before either the
fast-path or the iteration fallback runs, so non-numeric doc_ids
cannot reach the inner `c.id == int(doc_id)` comparison.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves both 🟡 important issues from the latest review:
1. `page_number` was unconditionally overwritten in `viz_routes.py:696` even
when Qdrant's payload lacked the field, clobbering the value resolved
from `chunk_context.page_number`. The new helper returns each field
independently and both call sites only overwrite via `is not None`
guards, matching the existing logic in `visualization.py`.
2. The ~60-line `if chunk_index is not None: ... else: ...` Qdrant scroll
block was duplicated between `api/visualization.py` and
`auth/viz_routes.py`. Extracted into `get_chunk_bbox_and_page_from_qdrant`
in `search/context.py` alongside the existing private `_get_chunk_*_from_qdrant`
helpers; both routes now share ~12 lines of caller code.
New unit tests at `tests/unit/test_chunk_bbox_helper.py` cover the indexed
and offset paths, the `(bbox, None)` regression case, and graceful
degradation on Qdrant strict-mode 400 (which also closes nit #4).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer feedback (2 items):
- Add a BOOL payload index for `is_placeholder` alongside the three
KEYWORD fields. Strict-mode index-required filtering on Qdrant Cloud
enforces a payload index on any field used in a `FieldCondition`
regardless of value type, so `get_placeholder_filter` and
`delete_placeholder_point` would have produced HTTP 400 on Cloud
instances even after this PR's KEYWORD fix.
Implementation: replace `_KEYWORD_PAYLOAD_FIELDS: tuple` with
`_PAYLOAD_INDEX_FIELDS: dict[str, PayloadSchemaType]` so each
field carries its own schema type. Rename
`_ensure_keyword_payload_indexes` to `_ensure_payload_indexes` since
the function now creates more than just KEYWORD indexes. The
per-field log line now includes the schema type
("Created KEYWORD payload index on 'doc_id'", "Created BOOL payload
index on 'is_placeholder'") so operators can tell which type was
created without checking the source.
- Correct the misleading `wait=True` docstring in
`_apply_backfill_writes`. The previous wording said
`_ensure_payload_indexes` runs "immediately after this function",
but `_apply_backfill_writes` is called in a loop inside
`_backfill_doc_id_to_string` — the index creation runs after the
backfill function *returns*, not after each write. Rewrote the
docstring to capture both load-bearing reasons:
(1) per-batch commit ordering for crash-recovery safety, and
(2) ensuring the keyword index built later covers committed
payloads only.
Adds `test_ensure_payload_indexes_includes_is_placeholder_as_bool`
asserting the schema type is BOOL specifically. Existing tests
updated to use the new dict-based registry (side_effect lists now
extend to all four entries; field-set assertions derive from the
registry instead of hardcoding 3 KEYWORD names).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`<d:href>` is required by RFC 3986 to be percent-encoded, so non-ASCII
filenames (e.g. Chinese, Cyrillic) were leaking through `list_directory`
and the SEARCH-based tools (`find_by_name`, `find_by_type`,
`list_favorites`, `search_files`) as their URL-encoded form. Decode with
the already-imported `urllib.parse.unquote` before exposing to callers.
Fixes#776
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer feedback (3 important + 3 nits):
- Wrap _ensure_keyword_payload_indexes' get_collection() call in
try/except. The qdrant_client singleton is already assigned by the
time this function runs, so a transient timeout/DNS failure
propagating out left the process holding a usable client with the
migration silently skipped on every subsequent call. Now logs ERROR
with exc_info and returns; next process restart retries.
- Add `and "doc_id" in point.payload` guard to the four set
comprehensions in scanner.py (indexed_doc_ids, indexed_file_ids,
indexed_item_ids, indexed_card_ids). Previously a payload missing
the doc_id key would raise KeyError and crash the entire scan.
- Tighten test_ensure_keyword_payload_indexes_logs_400_as_warning to
match the per-field warning prefix exactly (`startswith("Schema
conflict on payload index")`), so a future change adding 400s to
the partial-failure summary surfaces here as a count mismatch.
- Add new-collection vs existing-collection context to the
_backfill_doc_id_to_string docstring's `dimension` parameter.
- Replace the misleading "rewrote 0/N from int to str" wording when
no rewriting was needed with "N points scanned, none required
rewriting (collection already in str form)".
- Add test_ensure_keyword_payload_indexes_logs_and_returns_when_
get_collection_raises mirroring the scroll-failure test.
SonarCloud (1 CRITICAL + 1 MINOR):
- Refactor _backfill_doc_id_to_string to bring cognitive complexity
under 15 (was 19). Extracted two pure helpers: _group_int_doc_ids
(group point IDs by stringified doc_id) and _apply_backfill_writes
(apply set_payload calls and return rewritten count). The main
function's scroll/loop/sentinel structure is unchanged.
- Add `await asyncio.sleep(0)` to the three async test side_effect
helpers (_scroll_raises, _upsert_raises, _create_index) so they use
an actual async feature (S7503). The async-callable shape is still
required to avoid the AsyncMock unawaited-coroutine warning when
side_effect raises.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses three important findings from the latest reviewer comment:
- Add progress INFO log every 20 scroll batches (≈5120 points at
batch_size=256) in _backfill_doc_id_to_string so a long-running
migration on a large collection (50k+ points) doesn't look like a
startup hang. The line carries collection name, scanned count, and
rewritten count so it doubles as a heartbeat.
- Track non-400 failures in _ensure_keyword_payload_indexes and emit
a WARNING summary line listing every field that failed to get an
index. Per-field ERROR lines are easy to miss in startup noise; the
summary makes the partial-failure state visible at a glance.
- Split the sentinel upsert out of the data-scroll try/except in
_backfill_doc_id_to_string. A scroll-time failure still logs ERROR
with the new "scroll failed" wording (data is incomplete). A
sentinel-write failure now logs WARNING with "data succeeded but
sentinel write failed" wording — data is correct, only the
short-circuit marker is missing, and the next restart re-scrolls
an already-clean collection (idempotent zero-write) before retrying
the upsert.
Also fix the RuntimeWarning emitted by
test_backfill_logs_and_returns_when_scroll_raises: replace the bare
`RuntimeError` side_effect with an async-callable side_effect so
AsyncMock awaits the coroutine before the exception propagates.
Three new unit tests cover the new branches:
test_backfill_emits_progress_log_every_20_batches,
test_backfill_logs_warning_when_sentinel_upsert_fails,
test_ensure_keyword_payload_indexes_summarises_failed_fields.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Remove unreachable doc_type=="file" branch (and pymupdf/pymupdf4llm
imports) from _fetch_document_text in search/context.py — the file
path is short-circuited in get_chunk_with_context before reaching it.
- Drop the redundant `username = request.user.display_name` alias in
auth/viz_routes.py; both Qdrant scroll filters now reference user_id
consistently with the rest of the handler.
- Add TestAdjacentChunkBoundary in tests/unit/test_chunk_context_offset_gate.py
covering chunk_index=0 (before-fetch gate closed) and
chunk_index=total_chunks-1 (after-fetch gate closed) — the two
off-by-one boundaries previously untested.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Remove three stale `# Use numeric file ID` / `# Pass file path` comments
in scanner.py. file_id is already normalized to str() above each call
site, so the inline comments mislead readers.
- Wrap `_backfill_doc_id_to_string` scroll loop + sentinel upsert in
try/except Exception. The qdrant_client singleton is assigned before
this migration runs, so a transient scroll failure was leaving the
process holding a usable client with int payloads permanently
unbackfilled until the next restart. Catch broadly, log ERROR with
exc_info, and return without writing the sentinel — next process
restart retries from scratch.
- Note `:memory:` mode behavior near the sentinel constants so future
readers don't read the every-start scroll as a bug.
- Document the two degraded-migration ERROR log signals in
docs/configuration.md so operators know when a clean restart is
required to recover indexing.
- Add unit test asserting scroll-time exceptions are logged and swallowed
without writing the sentinel.
Closes round-4 review feedback on PR #773.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- pdf_highlighter.compute_chunk_bboxes_batch: drop unused chunk_text
destructure (SonarQube finding), and replace positional
page_boundaries[page_num - 1] with a key-based next() match so
reordered or non-1-indexed boundaries can't silently shift the bbox.
Convert touched f-string log to lazy %s formatting.
- vector/processor: rename the trace_operation span from
"vector_sync.generate_highlights" to "vector_sync.compute_chunk_bboxes"
to match what the function actually does.
- Add test_compute_chunk_bboxes_handles_unordered_page_boundaries —
reverses the boundaries list and asserts identical results to the
in-order case, guarding the boundary-lookup regression class.
- Pin pre-push-review skill to sonnet model.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add a fixed-UUID sentinel point written after a successful doc_id
backfill so subsequent restarts retrieve it and short-circuit the
O(N) scroll. Sentinel has no user_id/doc_id/doc_type payload so
production search filters never see it.
- Pre-fetch payload_schema in _ensure_keyword_payload_indexes and
silently skip fields that are already indexed; the "Created KEYWORD
payload index" INFO log fires only on actual creation.
- Narrow stale `int | str` doc_id annotations to `str` across
search/verification.py (BatchVerifier return type, per-verifier
accessible sets, by_type / accessible_by_type / inaccessible
collections); drop the now-redundant `type(d).__name__` prefix in
the dropped-docs log.
- Align the backfill log message with the PR description's
"Running doc_id backfill" promise; add a caller cross-reference to
the wait=True comment.
- Fix _get_file_path_from_qdrant docstring (file_id is str, not numeric).
- Convert legacy `id=1` to `id="1"` in test_search_result.py to match
the SearchResult.id: str annotation.
Three new unit tests cover sentinel-found, sentinel-written, and
skip-existing-index branches; existing backfill tests pass dimension
and explicit retrieve.return_value=[] for the no-sentinel path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- chunk_bboxes is now dict[int, list[tuple[...]]] holding the bbox list
directly, not {"bbox": ..., "page": ...}. The page from text-search was
stored but never read; page_number from offset-based assignment is
authoritative for the Qdrant payload.
- Add two unit tests for the documented omission contract: chunks whose
offsets fall outside every page boundary, and chunks whose text cannot
be located on the rendered page, are silently dropped from the result.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the deferred half of PR #767 review issue 2: instead of just
documenting the "0/N misreport" with a logger.warning, propagate the
caller's None for chunk_index through the dataclass, position markers,
and response builders so callers can distinguish "unknown position"
from "actually chunk 0".
Changes:
- ChunkContext.chunk_index: int → int | None
- _insert_position_markers: chunk_index parameter is int | None; when
None, renders "Chunk ?/N" instead of "Chunk 1 of N"
- get_chunk_with_context: drops the effective_chunk_index local entirely.
Passes chunk_index (may be None) directly into both ChunkContext and
_insert_position_markers, in both the Qdrant fast path and the
doc-text fallback.
- Fast path: when chunk_index is None and chunk_text was retrieved via
the offset lookup (notes/cards), skip the adjacent-chunk fetch.
Index arithmetic from a default 0 would query the chunks at positions
-1 and 1 even when the actual chunk is, say, 5/20 — silently producing
wrong "before"/"after" text. Mark both sides as truncated instead.
- Drop the now-redundant logger.warning in the doc-text fallback (the
response correctly communicates the unknown state via chunk_index=None).
Both existing response builders (`api/visualization.py:657` and
`auth/viz_routes.py:717`) already serialise `chunk_context.chunk_index`
unconditionally; `None` becomes JSON `null`. No route changes needed.
Adds 5 regression tests:
- ChunkContext.chunk_index propagates as None in fast path
- ChunkContext.chunk_index propagates as None in doc-text fallback
- Fast path with chunk_index renders "Chunk N of M" correctly
- _insert_position_markers renders "?/N" for None chunk_index
- _insert_position_markers renders explicit index when supplied
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- _ensure_keyword_payload_indexes: distinguish 400 (schema conflict, warning)
from other status codes (5xx/network, error) so a transient outage doesn't
silently leave the collection unindexed.
- build_search_result_from_point: use .get("doc_id") + return None on missing
instead of KeyError-crashing the search; reverse metadata merge order so
payload-derived chunk_index/total_chunks win over caller-supplied extras.
- docs/configuration.md: restore the OpenAI/Mistral/Bedrock/Simple provider
sections + reference-table rows that were dropped in the rebase. Reword
the "Startup migrations" bullet to describe what the code actually does
(no sampling — full scroll, zero writes when clean). Add operator note
about the SemanticSearchResult.id TypeError path.
- tests: pytest.approx for float equality (Sonar python:S1244); coverage
for non-400 → ERROR, payload={doc_id: None}, and missing doc_id key.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Drop chunk_bbox_page from Qdrant payload — viz endpoints never read it
(page_number is the canonical PDF page field).
- Bump upsert BATCH_SIZE 10 → 100 now that payloads no longer carry PNGs.
- compute_chunk_bboxes_batch: move doc.close() into finally, replace
unused stored_page_num with _.
- purge_page_images.py: switch to anyio.run() per project convention,
and wrap AsyncQdrantClient in try/finally so the aiohttp session is
always closed (the class doesn't implement async-context-manager).
- Decorate new bbox unit tests with @pytest.mark.unit so they run under
the fast-feedback selector.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Latest reviewer comment flagged five items on top of the original PR. This
commit addresses every one:
🟡 1. Skip the offset-based Qdrant fallback for `doc_type=file` when
`chunk_index` is supplied. Qdrant Cloud's strict mode rejects unindexed
filter fields with HTTP 400, which `_get_chunk_from_qdrant` catches and
logs at `logger.error` — masking real Qdrant problems in monitoring.
Notes/cards keep the offset fallback (cheap, useful for legacy data).
🟡 2. Add a `logger.warning` and clarifying inline comment in the doc-text
fallback path when `chunk_index` is None — surfaces the pre-existing
"0/N misreport" so callers can detect it. Type-nullability propagation
is deferred to a follow-up (out of scope for this hotfix).
🟢 3. Simplify `if chunk_text and doc_id_int is not None:` →
`if chunk_text:` with an inner `assert doc_id_int is not None` for
`ty` narrowing. The outer second clause was dead.
🟢 4. Add `doc_type` `FieldCondition` to the offset-based image lookup in
both `visualization.py` and `viz_routes.py` for parity with the
`chunk_index` branches.
🟢 5. Inline the `chunk_filter` local in `visualization.py` directly into
the `must=[]` list (matches `viz_routes.py` style).
Adds `tests/unit/test_chunk_context_offset_gate.py` with three regression
tests covering the gate matrix: (file, with-index → skip offset),
(note, with-index → still tries offset), (file, no-index → still tries
offset). Lives at top-level rather than `tests/unit/search/` to side-step
a pre-existing circular-init issue in `nextcloud_mcp_server.search`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-chunk PDF page renders (~150–700 KB base64 PNG each) were the dominant
disk consumer in production, repeatedly tripping `No space left on device:
WAL buffer size exceeds available disk space` on welcomed-malamute Qdrant.
Replace the inline highlighted_page_image / highlighted_page_number /
highlight_count fields with a small `chunk_bbox` field:
list[(x0, y0, x1, y1)] of normalized [0, 1] floats, ~32 bytes per chunk.
Astrolabe (the only known consumer) renders the highlight client-side as
a percentage-positioned overlay on top of the existing /api/v1/pdf-preview
render-on-demand path (cbcoutinho/astrolabe#76).
- pdf_highlighter: new compute_chunk_bboxes_batch() that reuses the
existing _find_chunk_bbox text-search path, skipping all pixmap/PIL/PNG
work.
- processor: store chunk_bbox + chunk_bbox_page in the Qdrant payload,
drop highlighted_page_image + friends, drop the base64 import.
- visualization /api/v1/chunk-context and auth/viz_routes: read
chunk_bbox instead of highlighted_page_image.
- vector/__init__: stop eagerly re-exporting `processor`/`scanner` —
fixes a pre-existing circular import (search.algorithms ->
vector.placeholder -> vector/__init__ -> processor -> scanner ->
server.semantic -> search.bm25_hybrid -> search.algorithms partial).
Test suite that was broken on master (test_bm25_hybrid.py et al.) now
collects and passes.
- scripts/purge_page_images.py: ad-hoc, idempotent migration that
delete_payload's the legacy keys from existing points. No reindex
required; legacy chunks render the page with no overlay.
Pairs with cbcoutinho/astrolabe#76. Frontend handles missing chunk_bbox
gracefully, so this can land in either order.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses reviewer feedback on PR #773:
- Backfill set_payload now uses wait=True to avoid a race where
_ensure_keyword_payload_indexes builds the KEYWORD index before
fire-and-forget writes have committed, leaving int payloads
invisible to filters.
- Batch points sharing the same int doc_id into a single set_payload
call (one document → many chunks → one round-trip instead of N).
- Drop _has_int_doc_id_sample short-circuit. The sample's false-negative
window (clean first 256 results, ints further in) is gone; full scroll
is the dominant cost on first run anyway.
- Simplify _ensure_keyword_payload_indexes: the "already exists" 400
branch was dead code (Qdrant returns 200 on identical re-create); any
400 now logs a warning and continues.
- search/context.py: comment the broadened file-type guard. Add explicit
not doc_id.isdigit() checks at the top of note/news_item/deck_card
branches in _fetch_document_text so malformed payloads surface as
warnings instead of being swallowed by the broad except.
Also extracts build_search_result_from_point into search/algorithms.py
to deduplicate the 71-line payload-extraction loop shared by
SemanticSearchAlgorithm and BM25HybridSearchAlgorithm. This fixes
SonarQube's quality-gate failure (4.0% new-code duplication, max 3%).
Test coverage:
- 7 new unit tests for build_search_result_from_point covering missing
payload, note/file/deck_card metadata, int doc_id coercion, and
metadata_extras merging.
- Replace _has_int_doc_id_sample tests with clean-collection no-op and
per-batch grouping tests.
- Update set_payload assertions from wait=False to wait=True.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add `doc_type` FieldCondition to the chunk_index-path highlighted-image
Qdrant filter in both `api/visualization.py` and `auth/viz_routes.py`,
matching the shape of `_get_chunk_by_index_from_qdrant`. Safe today (the
block is guarded by `doc_type == "file"` and Nextcloud file IDs are
globally unique) but prevents a latent bug if other doc types start
storing highlighted images.
- Demote `viz_routes.py` `ValueError` log from `error` to `warning` (lazy
%-style) — `_parse_int_param` raises on user-supplied bad input, which
is a 400 not a server error and shouldn't pollute error logs.
- Hoist `effective_chunk_index` to compute once at the top of
`get_chunk_with_context`, removing two duplicate assignments.
- Add `test_file_doc_type_qdrant_miss_yields_fast_404` to the management
endpoint tests, locking in the proxy-timeout fix contract.
- Add `tests/unit/test_viz_routes_chunk_context.py` mirroring management
coverage for the OAuth-session route: param forwarding (chunk_index /
total_chunks), `doc_type=file` fast 404, and 400 on invalid int params.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
