15a7e680a61827b07c0f4b35e465d845770260ae
31
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Chris CoutinhoandClaude Opus 4.7
|
ade42b55dc |
docs: clear review-round-3 nits — stale login_flow_v2, duplicates, field comments
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
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
282c245da1 |
refactor(config)!: drop ENABLE_MULTI_USER_BASIC_AUTH env var, fail loud on legacy aliases
Same pattern as the ENABLE_LOGIN_FLOW removal in the previous commit:
the deployment mode (MCP_DEPLOYMENT_MODE) is the single source of truth
for selecting an auth flow. The ENABLE_MULTI_USER_BASIC_AUTH env-var
alias is redundant with `MCP_DEPLOYMENT_MODE=multi_user_basic`.
Unlike the ENABLE_LOGIN_FLOW removal — where silent removal was safe
because Login Flow v2 is the auto-detection default — silent removal
here would be a surprise: a user with only ENABLE_MULTI_USER_BASIC_AUTH=true
in their .env would auto-detect into LOGIN_FLOW after upgrade (wrong
runtime mode). Mitigation: detect_auth_mode now reads os.environ
directly for both legacy aliases and raises ValueError with a one-line
migration message if either is set. Applied retroactively to
ENABLE_LOGIN_FLOW as well — loud is better than silent.
- nextcloud_mcp_server/config.py:
- Drop the dynaconf env-var alias entry for ENABLE_MULTI_USER_BASIC_AUTH.
- Update the `enable_multi_user_basic_auth` field docstring to mark it
as derived / not user-settable.
- `_is_multi_user_mode()` (early-config helper, runs before Settings
is built) switched to checking MCP_DEPLOYMENT_MODE directly. Now
consistent with the canonical detection in detect_auth_mode.
- nextcloud_mcp_server/config_validators.py:
- Drop the auto-detection branch (`if settings.enable_multi_user_basic_auth`).
Selection of MULTI_USER_BASIC is now exclusively via the explicit
MCP_DEPLOYMENT_MODE branch.
- Add `enable_multi_user_basic_auth` to `_sync_derived_flags` alongside
`enable_login_flow` — both flags are now derived from the resolved mode.
- Drop `enable_multi_user_basic_auth` from
`MODE_REQUIREMENTS[MULTI_USER_BASIC].required` and from the
`forbidden` lists of SINGLE_USER_BASIC and LOGIN_FLOW (no longer
user input → no meaningful forbidden check).
- Add loud-deprecation `ValueError` block at the top of detect_auth_mode
that errors with a clear migration message when ENABLE_MULTI_USER_BASIC_AUTH
or ENABLE_LOGIN_FLOW is found in os.environ.
- tests/unit/test_config_validators.py:
- Switch ~10 fixtures from `enable_multi_user_basic_auth=True` to
`deployment_mode="multi_user_basic"` (mirrors `enable_login_flow`
treatment from the previous commit).
- Switch two `patch.dict(os.environ, {"ENABLE_MULTI_USER_BASIC_AUTH": "true"})`
blocks to use MCP_DEPLOYMENT_MODE.
- Rename `test_forbidden_multi_user_basic_auth` to
`test_forbidden_multi_user_basic_when_credentials_present` — the
scenario is now an explicit-mode + credentials conflict, not an
env-var-flag conflict.
- Add `test_legacy_enable_multi_user_basic_auth_env_var_errors` and
`test_legacy_enable_login_flow_env_var_errors` to exercise the new
loud-deprecation ValueError path.
- docker-compose.yml: mcp-multi-user-basic profile switched to
`MCP_DEPLOYMENT_MODE=multi_user_basic`.
- env.sample: replaced `#ENABLE_MULTI_USER_BASIC_AUTH=true` example with
`#MCP_DEPLOYMENT_MODE=multi_user_basic`.
- docs/authentication.md, configuration.md, troubleshooting.md,
auth-flows.md, webhook-management-guide.md,
configuration-migration-v2.md, ADR-025: replaced env-var examples
with the canonical MCP_DEPLOYMENT_MODE form.
- docs/ADR-020: marked partly superseded by ADR-022.
- CLAUDE.md: Multi-User BasicAuth section updated to set
MCP_DEPLOYMENT_MODE.
- nextcloud_mcp_server/vector/oauth_sync.py: module docstring updated.
BREAKING CHANGE: ENABLE_MULTI_USER_BASIC_AUTH is no longer read from
the environment, and setting it now raises a startup ValueError with
a migration message. Replace `ENABLE_MULTI_USER_BASIC_AUTH=true` with
`MCP_DEPLOYMENT_MODE=multi_user_basic`. The same loud-deprecation
check is also applied to the recently-removed ENABLE_LOGIN_FLOW —
replace with `MCP_DEPLOYMENT_MODE=login_flow` (or drop both;
`login_flow` is the auto-detect default when no other auth env vars
are set).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
df4994e860 |
refactor(config)!: derive enable_login_flow from mode, remove ENABLE_LOGIN_FLOW env var
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>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
8246d9a088 |
fix(vector): address PR review round 17 + local-mode collection-creation regression
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> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
b97ac23228 |
fix(vector): address PR review round 4 — backfill resilience + degraded-mode docs
- 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> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
b5b4025bb4 |
fix(vector): address PR review round 2 — status branching, doc_id guard, doc restore
- _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>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
719b3b5034 |
fix(vector): normalize doc_id to str + add Qdrant keyword payload indexes
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>
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
22ed9e99a0 |
feat(webdav): add tag-based file exclusion (#710)
Hide sensitive files/folders from the WebDAV MCP tool surface by tagging them with a configured Nextcloud system tag. Defence-in-depth control for users who connect LLMs to accounts holding contracts, medical records, credentials, etc. A new EXCLUDED_TAGS env var (comma-separated tag names, empty by default) gates an exclusion layer that runs at the start of every WebDAV tool call: tag names are resolved to tag IDs, those IDs are expanded to the set of tagged paths, then listings/searches are filtered and read/write/delete/move/copy operations on excluded paths raise ToolError. Tagged folders exclude their descendants via prefix match. Empty EXCLUDED_TAGS disables the feature entirely. The threat model is preventing accidental data exfiltration via the LLM tool surface — not hiding files from a determined operator. The docs explicitly recommend creating exclusion tags with user_assignable=false so the credentials the MCP server uses cannot remove the tag. Implementation: - config.py: add `excluded_tags` to _DEFAULTS, Settings, and the _field_map alongside other comma-separated env vars. - client/webdav.py: get_files_by_tag now requests <d:resourcetype/> and surfaces is_directory so tagged directories can recursively exclude descendants. - server/tag_exclusion.py (new): get_excluded_tag_names, get_excluded_file_paths, is_path_excluded. - server/webdav.py: exclusion guards in all 11 WebDAV tools; read/write/create/delete/move/copy raise ToolError, list/search tools silently filter excluded entries. Existing f-string log calls converted to lazy %-style. - tests: 17 new unit tests covering path-matching edge cases (shared-prefix non-match, descendants of excluded dirs), tag-name parsing, and get_excluded_file_paths with mocked WebDAV; 1 new client test asserting <d:resourcetype/> -> is_directory parsing. - docs/configuration.md: new "Tag-Based File Exclusion" section with per-tool effect table, security guidance, and per-call cost note. - README.md: feature mention under Key Features. Closes #710. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
8a2626da6c |
refactor(search): address PR #750 round 8 review feedback
- Rename `verified_count` → `verified_chunk_count` to make the count granularity explicit at the field name (chunks vs unique docs). - News verifier now fails open *per-item* on non-numeric stored doc_ids (matches notes/files/deck shape); a single bad id no longer rescues definitively-missing siblings from eviction. - Update note-verifier integration test to use string doc_ids end-to-end to match production storage (scanner.py:241 stringifies note ids). - Add regression test for the closed-task-group race guard in `verify_search_results` so the RuntimeError swallow is locked in. - Convert remaining f-string logger calls in `server/semantic.py` to lazy %-style formatting (per repo convention). - Document `evict_on_missing` as a developer/test flag (no env var) and flag the `get_file_info` 404→raise contract change in its docstring. - Add a TODO(ADR-019) breadcrumb for the hardcoded 2× over-fetch so future tuning has a clear hook. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
e8df6003c5 |
refactor(search): address PR #750 round 6 review feedback
Closes out the remaining nits flagged in the round-6 review. Critical: - _verify_files contract comment now enumerates all None-return cases (404 + malformed PROPFIND XML) and documents the false-eviction trade-off; self-healing via re-indexing recovers - int(r.id) cast at the SemanticSearchResult boundary now raises a TypeError with explicit doc_type/value context instead of bubbling up as an opaque "Search failed: ..." McpError Design observations: - nc_semantic_search_answer docstring documents the per-note round-trip cost from the post-verification race guard - News verification latency hint added to configuration.md - SemanticSearchResponse exposes verified_count + dropped_count so short result pages on high-ghost-density indexes are distinguishable from genuine scarcity. verify_search_results now returns (kept, dropped_count); production caller and tests updated Minor: - Comment clarifies the .get() fallback in verify_search_results is defensive only (run_verifier always populates the entry) - Eviction task-group guard narrowed from except Exception to except RuntimeError (the only documented failure mode of TaskGroup.start_soon on a closed group) - Indexer logs a warning when a deck_card task is missing board_id/stack_id, surfacing data-quality issues at index time rather than at verification time - New unit test covers the news verifier's non-numeric-id fail-open path (one bad doc_id keeps the entire batch) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
ffcca23a7b |
refactor(search): address PR #750 round 5 review feedback
Tightens verifier consistency, closes test gaps, hardens the fire-and-forget eviction snapshot, and routes the new concurrency knob through Settings. - Pre-flight ``int()`` guard in ``_verify_notes`` mirrors ``_verify_deck_cards``, so a non-numeric note id produces a type-specific log line instead of falling through to the generic "unexpected error" branch. - Adds explicit 403 tests for the file and news verifiers (symmetry with the existing notes/deck 403 tests) plus a ``non_numeric_id_keeps`` test. - ``AppContext`` and ``OAuthAppContext`` no longer snapshot ``_vector_sync_state.eviction_task_group`` at lifespan-yield time. Both expose it as a ``@property`` that reads the singleton dynamically, removing the order-sensitive race where a future startup-ordering change could silently degrade fire-and-forget eviction to inline forever. - Adds ``verification_concurrency`` (env var ``VERIFICATION_CONCURRENCY``, default 20) to ``Settings`` with a dynaconf validator; ``verify_search_results`` resolves the cap lazily from settings when the caller doesn't override it. - Enriches the news verifier TODO to call out that ``batch_size=-1`` is intentional — a numeric ceiling would silently break correctness because any item beyond the cap would be missing from ``present_ids`` and dropped. - Updates ``Optional[TaskGroup]`` to ``TaskGroup | None`` per project style. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
21e5608a39 |
refactor(search): address PR #750 round 2 review feedback
Implements fire-and-forget eviction (ADR-019 §"Lazy eviction"): the search response no longer waits on Qdrant deletes, instead spawning evict() on a long-lived lifespan-owned task group. Falls back to inline eviction in modes without vector sync and in unit tests. Also: harden _verify_news_items against non-numeric ids (fail open instead of crashing the verifier); document the get_file_info None-on-404 contract; add INDEXED_DOC_TYPES single source of truth in vector/scanner.py referenced by the CI-guard test; write a Verify-on-Read Latency Budget section in docs/configuration.md covering the unbounded news.get_items fetch. Closes the two remaining ADR-019 implementation checklist items. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
0c6b766e7e |
docs: fix scope naming and round-3 reviewer feedback
The docs claimed scopes are mcp:-prefixed (mcp:notes.read, mcp:notes.write) and that the notes.* pair "covers all Nextcloud apps". Both are false. Per @require_scopes decorators across nextcloud_mcp_server/server/, scopes are unprefixed and per-app: notes.read/write, talk.read/write, files.read/write, calendar.read/write, contacts.read/write, deck.read/write, news.read, tables.read/write, cookbook.read/write, todo.read/write, collectives.read/write, sharing.write, semantic.read, plus standard OIDC scopes. Changes: - login-flow-v2.md: replace the false 2-row "covers all apps" scope table with the real per-app reference (links to scope_authorization.discover_all_scopes() as authoritative source); strip mcp: prefix from intro paragraph, sequence diagrams, @require_scopes example, WWW-Authenticate header example. Also fix sticky-session keying advice per reviewer: route on user identity (sub claim) rather than the raw bearer token, since tokens rotate on refresh. - auth-flows.md: clarify "Astrolabe (hosted UI) → MCP" matrix column header; strip mcp: from sequence diagram and key characteristics bullet; correct "issued by MCP server" to "issued by configured IdP" on the Login Flow v2 token. - authentication.md: strip mcp: from the high-level diagram and scope-enforcement prose; cross-link to the scope reference. - configuration.md: add NEXTCLOUD_OIDC_CLIENT_ID, NEXTCLOUD_OIDC_CLIENT_SECRET, and OIDC_DISCOVERY_URL to the Login Flow v2 vars table — these were undocumented in the table after the round-2 multi-IdP fix. - running.md: drop deprecated `version: '3.8'` from compose snippets (Compose v2 ignores it and emits warnings). - testing-oidc-consent.md: fix sample authorize URL and consent description to use real scope names instead of mcp:-prefixed ones (the manual test as written would have failed with invalid_scope). - CLAUDE.md: replace dead links to deleted oauth-architecture.md, oauth-setup.md, and audience-validation-setup.md with login-flow-v2.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
d21be6d5e1 |
docs: generalize OIDC framing to support multiple IdPs
Previous round narrowed the framing too far in the other direction —
made it sound like Nextcloud OIDC is *the* IdP. The MCP server
actually supports any OIDC-compliant provider (Nextcloud's built-in
OIDC, Keycloak, AWS Cognito, Auth0, etc.) selected via
`OIDC_DISCOVERY_URL`. `NEXTCLOUD_OIDC_CLIENT_ID/SECRET` are generic
OIDC client credentials despite the Nextcloud-flavored naming.
Code references:
- IdP discovery: app.py:607-668 (auto-detects integrated vs external
by comparing discovered issuer to NEXTCLOUD_HOST)
- JWKS: unified_verifier.py:71-73 (dynamically discovered, not
hard-coded to Nextcloud)
- IdP selection knob: OIDC_DISCOVERY_URL (config.py)
Changes:
- login-flow-v2.md: redraw "How It Works" diagram to show the IdP as
a separate component; replace "Nextcloud OIDC" with "configurable
IdP" framing throughout; add OIDC_DISCOVERY_URL to the env-var
reference; clarify NEXTCLOUD_OIDC_CLIENT_ID/SECRET are generic OIDC
creds; rename "OAuth Endpoints" subtitle to point at "the configured
IdP".
- running.md: rewrite the OAuth Mode intro and Quick Start note to
mention IdP configurability and OIDC_DISCOVERY_URL.
- configuration.md: update Best Practices "For Production" multi-user
bullet to reference the IdP selector and generic-creds caveat.
- auth-flows.md: generalize Astrolabe-flow and Login Flow v2
characteristics bullets — IdP and JWKS source are configurable.
- keycloak-multi-client-validation.md: REMOVE the "deprecated"
banner I added in
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
319e82774e |
docs: correct OIDC architecture framing for Login Flow v2
The previous round of review feedback rested on a misunderstanding — that the MCP server is "the OAuth issuer" under Login Flow v2 and that NEXTCLOUD_OIDC_CLIENT_ID/SECRET are external-IdP-only. Code says otherwise (app.py:619/625/703-717, unified_verifier.py:72): - The MCP server is an OIDC relying party of Nextcloud OIDC. Tokens are signed by Nextcloud and validated against Nextcloud's JWKS in all modes — the server has no private signing keys. - Static NEXTCLOUD_OIDC_CLIENT_ID/SECRET are the preferred way to register the MCP server as that relying party; RFC 7591 DCR is a fallback when both are unset. - Login Flow v2 layers per-user app-password acquisition on top — it governs the MCP→Nextcloud data leg, not the relying-party setup. This commit reverts the inaccuracies introduced by |
||
|
Chris CoutinhoandClaude Opus 4.7
|
35c115ead6 |
docs: address remaining Login Flow v2 review feedback
Round-2 cleanup of PR #743 review comments not covered by
|
||
|
Chris CoutinhoandClaude Opus 4.7
|
d153e96520 |
docs: address Login Flow v2 review feedback
Fix issues raised by reviewer on PR #743: - troubleshooting.md: renumber "Getting Help" steps (4→3, 5→4) after earlier consolidation left a gap - installation.md: drop stale "OIDC app" prerequisite; admin access is now optional under Login Flow v2 (works on stock Nextcloud 16+) - semantic-search-architecture.md: rename VECTOR_SYNC_ENABLED to ENABLE_SEMANTIC_SEARCH in the Status callout (renamed in v0.58.0) - configuration.md: remove Quick Start references to deprecated oauth-multi-user / oauth-advanced templates and point to login-flow-v2.md; update "OAuth, Multi-User BasicAuth" label to "Login Flow v2, Multi-User BasicAuth" - auth-flows.md: fix background-sync diagram so Encrypt+persist step no longer crosses into the Nextcloud column Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
1306849353 |
docs: pivot to Login Flow v2; add Astrolabe Cloud hosted offering
Replace the seven OAuth-to-Nextcloud docs (oauth-setup, quickstart-oauth, oauth-architecture, oauth-upstream-status, oauth-troubleshooting, jwt-oauth-reference, audience-validation-setup) with a single new docs/login-flow-v2.md. The deprecated flow required upstream user_oidc patches that were never merged; Login Flow v2 is the forward-looking multi-user mode (see ADR-022), and works with stock Nextcloud 16+. Rewrite docs/authentication.md and docs/auth-flows.md around three modes: Single-User BasicAuth, Multi-User BasicAuth pass-through, and Login Flow v2. Update README to add an Astrolabe Cloud (https://astrolabecloud.com) callout for users who prefer not to self-host, drop the OAuth deployment mode from the auth table, simplify the Docker block, and trim the Examples and Security sections. Sweep configuration.md, installation.md, troubleshooting.md, running.md, and semantic-search-architecture.md to replace links to the deleted docs and update deprecated mode names. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.6
|
cc6ba65993 |
fix: address second round of PR review for scope prefix
- Use dynaconf (get_settings()) instead of os.getenv for OIDC_RESOURCE_SERVER_ID
- Re-add Settings field, _field_map entry, and settings.toml default
- Add trailing-slash guard (.rstrip("/")) to prevent double-slash in scopes
- Add double-prefixing guard: skip scopes already carrying the prefix
- Add @pytest.mark.unit to test module
- Add test for already-prefixed scopes
- Document OIDC_RESOURCE_SERVER_ID in docs/configuration.md
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude Opus 4.6
|
7956c3c061 |
refactor: remove Smithery deployment mode
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> |
||
|
Chris CoutinhoandClaude Opus 4.6
|
1707b2e6e1 |
feat: add self-signed SSL certificate support for Nextcloud connections
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> |
||
|
Chris CoutinhoandClaude Opus 4.6
|
08d37a6597 |
docs: clean up astrolabe references after extraction
Remove astrolabe-specific docs and sections that belong in the astrolabe repo. Update remaining references to point to the astrolabe repo where appropriate. - Fix .gitmodules SSH → HTTPS URL for astrolabe submodule - Remove bump-version.yml stale "astrolabe" scope comment - Delete blog-introducing-astrolabe.md (moved to astrolabe repo) - Remove "Astrolabe Background Token Refresh" section from auth-flows.md - Replace "Astrolabe User Setup" section in authentication.md with link - Remove "Astrolabe Internal URL" section from configuration.md - Remove "Webhook Presets (via Astrolabe UI)" from webhook guide Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.5
|
c018268681 |
docs(astrolabe): add config docs and unit tests for internal URL
Address PR #487 reviewer feedback: - Add documentation for `astrolabe_internal_url` config option - Add unit tests for `IdpTokenRefresher::getNextcloudBaseUrl()` - Fix CI workflow paths (astroglobe -> astrolabe) - Add PHPUnit job to CI workflow for PHP 8.1, 8.2, 8.3 - Remove obsolete ApiTest that tested non-existent method Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Sonnet 4.5
|
1a5bb10cd0 |
feat(config): consolidate configuration with smart dependency resolution (ADR-021)
Simplifies configuration by consolidating overlapping settings and adding automatic dependency resolution. This makes semantic search configuration significantly easier for users while maintaining 100% backward compatibility. ## Key Changes ### Variable Renaming (Backward Compatible) - `VECTOR_SYNC_ENABLED` → `ENABLE_SEMANTIC_SEARCH` (old name still works) - `ENABLE_OFFLINE_ACCESS` → `ENABLE_BACKGROUND_OPERATIONS` (old name still works) - Deprecation warnings logged when old names used - Old names will be removed in v1.0.0 ### Smart Dependency Resolution - `ENABLE_SEMANTIC_SEARCH` automatically enables background operations in multi-user modes - No need to set both `ENABLE_OFFLINE_ACCESS` and `VECTOR_SYNC_ENABLED` anymore - Single-user mode doesn't auto-enable background ops (not needed) ### Explicit Mode Selection (Optional) - New `MCP_DEPLOYMENT_MODE` environment variable - Valid values: single_user_basic, multi_user_basic, oauth_single_audience, oauth_token_exchange, smithery - Removes ambiguity about which deployment mode is active - Falls back to auto-detection if not set (existing behavior) ### Configuration Templates - Reorganized `env.sample` by deployment mode with clear sections - Added mode-specific quick-start templates: - `env.sample.single-user` - Simplest configuration - `env.sample.oauth-multi-user` - Recommended multi-user - `env.sample.oauth-advanced` - Token exchange mode ## Implementation Details ### Files Modified - `nextcloud_mcp_server/config.py` - Smart dependency resolution helpers - `nextcloud_mcp_server/config_validators.py` - Simplified validation, explicit mode - `tests/unit/test_config_validators.py` - 19 new tests (60 total, all passing) - `env.sample` - Reorganized by deployment mode - `docs/configuration.md` - Complete rewrite with consolidated approach - `docs/troubleshooting.md` - New consolidation troubleshooting section - `README.md` - Updated variable references ### New Files - `docs/ADR-021-configuration-consolidation.md` - Architecture decision record - `docs/configuration-migration-v2.md` - Comprehensive migration guide - `env.sample.single-user` - Single-user quick-start template - `env.sample.oauth-multi-user` - OAuth multi-user quick-start template - `env.sample.oauth-advanced` - Token exchange quick-start template ## User Impact ### Before (Confusing) ```bash ENABLE_OFFLINE_ACCESS=true # Why both? VECTOR_SYNC_ENABLED=true # What's the relationship? ``` ### After (Simplified) ```bash MCP_DEPLOYMENT_MODE=oauth_single_audience # Explicit (optional) ENABLE_SEMANTIC_SEARCH=true # Auto-enables background ops! ``` ### Benefits - 📉 2 fewer variables to understand for semantic search - 📋 Clear intent ("I want semantic search") - 🎯 Explicit mode declaration available - 🔄 100% backward compatible - ✅ All 265 unit tests passing ## Testing - All 60 config validation tests passing - 10 new tests for configuration consolidation - 9 new tests for explicit mode selection - Full unit test suite: 265 tests passing - Backward compatibility verified ## Migration Users can migrate at their own pace. Old variable names continue working with deprecation warnings. See docs/configuration-migration-v2.md for detailed migration instructions. Related: ADR-021 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude
|
cb39b3fca4 |
feat(vector): Add configurable chunk size and overlap for document embedding
Enable users to tune document chunking parameters to match their embedding model and content type by adding DOCUMENT_CHUNK_SIZE and DOCUMENT_CHUNK_OVERLAP environment variables. - **config.py**: Added `document_chunk_size` (default: 512) and `document_chunk_overlap` (default: 50) configuration fields with validation: - Ensures overlap < chunk_size - Warns if chunk_size < 100 words - Prevents negative overlap values - **processor.py**: Updated DocumentChunker instantiation to use config settings instead of hardcoded values (line 174-177) - **tests/unit/test_config.py**: Added TestChunkConfigValidation class with 9 tests covering: - Default values - Valid configurations - Validation errors (overlap >= chunk_size, negative overlap) - Warning for small chunk sizes - Environment variable loading - **docs/configuration.md**: Added comprehensive "Document Chunking Configuration" section with: - Chunk size selection guidance (256-384 vs 512 vs 768-1024 words) - Overlap recommendations (10-20% of chunk size) - Configuration examples for different use cases - Added env vars to reference table - **docs/semantic-search-architecture.md**: Added "Document Chunking Strategy" section with: - Chunking process explanation - Example showing sliding window behavior - Search behavior with chunks - Tuning recommendations - **env.sample**: Added complete "Semantic Search & Vector Sync Configuration" section with: - Vector sync settings - Qdrant configuration (3 modes) - Ollama embedding service - Document chunking configuration - **docker-compose.yml**: Added commented examples for DOCUMENT_CHUNK_SIZE and DOCUMENT_CHUNK_OVERLAP with usage notes \`\`\`bash DOCUMENT_CHUNK_SIZE=512 DOCUMENT_CHUNK_OVERLAP=50 \`\`\` 1. \`overlap\` must be less than \`chunk_size\` 2. \`overlap\` cannot be negative 3. Warning issued if \`chunk_size\` < 100 words **Precise matching** (small notes, specific queries): \`\`\`bash DOCUMENT_CHUNK_SIZE=256 DOCUMENT_CHUNK_OVERLAP=25 \`\`\` **Balanced** (default, general purpose): \`\`\`bash DOCUMENT_CHUNK_SIZE=512 DOCUMENT_CHUNK_OVERLAP=50 \`\`\` **Contextual** (long documents, broader topics): \`\`\`bash DOCUMENT_CHUNK_SIZE=1024 DOCUMENT_CHUNK_OVERLAP=100 \`\`\` ✅ **User control** - Tune chunking to match embedding model capabilities ✅ **Experimentation** - Test different chunk sizes for optimal results ✅ **Model alignment** - Match chunk size to embedding context window ✅ **Backward compatible** - Defaults maintain existing behavior ✅ **Well validated** - Comprehensive tests prevent misconfiguration All 22 config validation tests pass (9 new tests for chunking): - Default values work correctly - Validation prevents invalid configurations - Environment variables load properly - Warning system works as expected With configurable chunk sizes, users can now experiment with different Ollama embedding models and tune chunk parameters for optimal semantic search quality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude
|
e575c8e57b |
feat(vector): Support multiple embedding models with auto-generated collection names
This PR enables safe switching between embedding models and multi-server
deployments by implementing auto-generated Qdrant collection names based on
deployment ID and model name.
## Problem
Previously, all deployments used a single hardcoded collection name
"nextcloud_content", which caused two critical issues:
1. **Dimension mismatches when switching models**: Changing
OLLAMA_EMBEDDING_MODEL (e.g., nomic-embed-text at 768D → all-minilm at
384D) would cause runtime errors as vectors couldn't be inserted into a
collection with incompatible dimensions.
2. **Collection collisions in multi-server setups**: Multiple MCP servers
sharing a single Qdrant instance would overwrite each other's data,
making horizontal scaling impossible.
## Solution
### Auto-Generated Collection Naming
Collections are now automatically named using the pattern:
\`{deployment-id}-{model-name}\`
**Deployment ID**: Uses \`OTEL_SERVICE_NAME\` if configured (and not default
value), otherwise falls back to \`hostname\` for simple Docker deployments.
**Model Name**: From \`OLLAMA_EMBEDDING_MODEL\` with path separators sanitized.
**Examples**:
- \`my-mcp-server-nomic-embed-text\` (with OTEL_SERVICE_NAME=my-mcp-server)
- \`mcp-container-all-minilm\` (simple Docker, hostname=mcp-container)
**Override**: Users can still set \`QDRANT_COLLECTION\` explicitly to bypass
auto-generation for backward compatibility.
### Dimension Validation
Added startup validation that checks collection dimensions match the
embedding service. If a mismatch is detected, the server fails fast with a
clear error message explaining:
- Expected vs actual dimensions
- Likely cause (model change)
- Solutions (delete collection, use different name, or revert model)
### Improved Sampling Error Handling
Enhanced MCP sampling rejection handling to treat user rejections as normal
behavior rather than errors:
- **User rejections** ("rejected", "denied") → INFO log, no traceback
- **Unsupported clients** → INFO log, no traceback
- **Other MCP errors** → WARNING log, no traceback
- **Unexpected errors** → ERROR log WITH traceback
This aligns with the MCP specification where clients SHOULD prompt users for
approval/denial of sampling requests.
## Changes
### Core Implementation
- **nextcloud_mcp_server/config.py**: Added \`get_collection_name()\` method
with deployment ID detection and model name sanitization
- **nextcloud_mcp_server/vector/qdrant_client.py**: Dimension validation on
collection open with helpful error messages
- **nextcloud_mcp_server/vector/{scanner,processor}.py**: Updated to use
\`get_collection_name()\`
- **nextcloud_mcp_server/auth/userinfo_routes.py**: Vector sync status uses
\`get_collection_name()\`
- **nextcloud_mcp_server/server/semantic.py**:
- Updated semantic search tools to use \`get_collection_name()\`
- Improved sampling rejection error handling (McpError vs Exception)
### Documentation
- **docs/semantic-search-architecture.md**: New comprehensive architecture
document (557 lines) covering background sync, semantic search flow, RAG
implementation, and deployment modes
- **docs/configuration.md**: Added detailed "Qdrant Collection Naming"
section with examples and multi-server deployment guidance
- **docker-compose.yml**: Added comments explaining collection naming behavior
- **README.md**: Updated semantic search descriptions to clarify
experimental status, Notes-only support, and infrastructure requirements
## Migration Guide
**For existing single-server deployments:**
Option 1 (Recommended): Use explicit collection name for continuity
\`\`\`bash
QDRANT_COLLECTION=nextcloud_content # Keep existing collection
\`\`\`
Option 2: Allow auto-generation and re-embed
\`\`\`bash
# Remove QDRANT_COLLECTION override
# New collection will be created based on deployment ID + model
# Requires re-embedding all documents (may take time)
\`\`\`
**For new multi-server deployments:**
Set unique OTEL service names per server:
\`\`\`bash
# Server 1
OTEL_SERVICE_NAME=mcp-prod
OLLAMA_EMBEDDING_MODEL=nomic-embed-text
# → Collection: "mcp-prod-nomic-embed-text"
# Server 2
OTEL_SERVICE_NAME=mcp-staging
OLLAMA_EMBEDDING_MODEL=nomic-embed-text
# → Collection: "mcp-staging-nomic-embed-text"
\`\`\`
## Benefits
✅ **Safe model switching**: Each model gets its own collection, preventing
dimension mismatch errors
✅ **Multi-server support**: Multiple MCP servers can share one Qdrant
instance without conflicts
✅ **Clear ownership**: Collection names show which deployment and model owns
the data
✅ **Better error messages**: Dimension validation provides actionable
guidance
✅ **Backward compatible**: Existing deployments can continue using
\`QDRANT_COLLECTION\` override
## Testing
Validated with:
- Single-server deployments (default hostname-based naming)
- Multi-server deployments (OTEL service name-based naming)
- Model switching scenarios (dimension validation)
- Collection override scenarios (backward compatibility)
Next steps: Testing various Ollama embedding models to investigate optimal
chunk sizes and performance characteristics.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude
|
857d8f2152 |
feat: add Qdrant local mode support with in-memory and persistent storage
Adds flexible Qdrant deployment modes to reduce infrastructure requirements
for local development and smaller deployments:
**Configuration Changes:**
- Add QDRANT_LOCATION environment variable (mutually exclusive with QDRANT_URL)
- Three modes: network (URL), in-memory (:memory:, default), persistent (file path)
- Settings dataclass validation via __post_init__ ensures mutual exclusivity
- API key warning when set in local mode (ignored, only for network mode)
**Client Initialization:**
- Auto-detect mode: network (url + api_key) vs local (:memory: or path=)
- In-memory: AsyncQdrantClient(":memory:") - zero config default
- Persistent: AsyncQdrantClient(path="/app/data/qdrant") - file storage
- Network: AsyncQdrantClient(url, api_key) - production mode
**Docker Compose Updates:**
- Qdrant service moved to optional profile (--profile qdrant)
- MCP service uses QDRANT_LOCATION=:memory: by default
- Added mcp-data volume for persistent storage (/app/data)
- No hard dependency on qdrant service
**Documentation:**
- Comprehensive configuration guide in docs/configuration.md
- All three modes documented with pros/cons
- Docker Compose examples for each mode
- Environment variable reference table
**Tests:**
- 13 new config validation tests (mutual exclusivity, defaults, warnings)
- Persistent mode integration test (create, close, reopen, verify persistence)
- All 82 unit tests + 5 smoke tests pass
**Breaking Change:**
- Default changed from QDRANT_URL=http://qdrant:6333 to QDRANT_LOCATION=:memory:
- Simplifies local development (no external service needed)
- Production deployments: explicitly set QDRANT_URL or QDRANT_LOCATION
Related: ADR-007 background vector sync implementation
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
|
||
|
Chris CoutinhoandClaude
|
2ca6725fc6 |
docs: Replace .nextcloud_oauth_client.json references with SQLite storage
Replace all references to the JSON file-based OAuth client storage with SQLite database storage in documentation. OAuth client credentials are now stored in the SQLite database instead of .nextcloud_oauth_client.json. Changes: - Update oauth-architecture.md to reference SQLite database - Update jwt-oauth-reference.md credential storage sections - Update oauth-setup.md Docker volume mounts and security best practices - Update oauth-troubleshooting.md file permission → database permission errors - Update configuration.md to remove JSON file chmod instructions - Update troubleshooting.md database permission troubleshooting The code already uses SQLite (RefreshTokenStorage class), so only documentation needed updating. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> |
||
|
Chris Coutinho
|
3ed24bd5e3 | docs: restructure documentation | ||
|
Chris Coutinho
|
4b19964817 | docs: Update docs | ||
|
Chris Coutinho
|
2489a714b8 | docs: Update README and docs |