f8afde07510abd890ea9ed59b6ac67be14ec5a5f
36
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Chris CoutinhoandClaude Opus 4.8
|
b5ed1e3b4d |
fix: address PR #814 review + SonarCloud gate
SonarCloud: - Resolve 6 S5332 hotspots (http→https in test fixture URLs). - S6418: hoist the unauthenticated AsyncOpenAI placeholder to a named constant + NOSONAR (genuine non-secret; gateway ignores it when unauthenticated). - Fix two reliability bugs: None-index guard in the gateway token-cache test (S2259) and float `> 0.0` instead of `!= 0.0` in the sentinel test (S1244). - status.py idle path sleeps 0.1s instead of sleep(0) (S7491); NOSONAR on the protocol-required async no-await aclose() stubs (S7503). Claude review: - Remove three leftover debug print() calls in app.py (logger.info already covers them). - payload_backfill: drop parsed_at from the backfilled-keys docstring (it is per-document state, not a deployment scalar); add a clean 404 precondition for BasicAuth deployments without an OAuth token verifier. - status.py: task_status typed TaskStatus | None (drop type: ignore). - nats.py: TODO to thread etags for file/deck/news; note etag default → None. - factory: warn on unknown INGEST_BUS_URL scheme; raise ValueError instead of assert for the external-mode preconditions. - docs/configuration.md: document the decomposition hook-point env vars + that nats-py ships core (lazy-imported) and external+bus uses two NATS connections. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
e98903c502 |
fix(storage): address review on PR #799 (stale comments, docs deprecation, unit test)
claude-review on #799 flagged: 1. Stale inline comment in ``initialize()`` (line 466) still said "Postgres uses a small bounded pool". Updated to reflect both backends now use NullPool. 2. Stale ``close()`` docstring referenced pool-size starving max_connections — irrelevant with NullPool. Replaced with the NullPool-aware rationale (dispose still tears down in-flight asyncpg connections cleanly). 3. ``docs/configuration.md`` actively directed operators to tune DATABASE_POOL_SIZE / DATABASE_MAX_OVERFLOW, with worked examples and pool math. Both are now deprecated no-ops; the table entries explain the deprecation and link to PR #799. Operators reading the docs will no longer be confused into tuning settings that don't do anything. 4. ``config.py`` comment for the deprecated fields updated to record the deprecation. Validators are intentionally kept (still reject < 1 / < 0) so misconfigured deploys fail loudly rather than silently — the reviewer flagged this as a minor UX wart but explicitly "not a blocker"; the docs change in (3) keeps operators away from the config altogether. 5. New ``tests/unit/test_storage_engine.py`` with three tests: - ``test_postgres_engine_uses_nullpool`` — pins ``isinstance( engine.pool, NullPool)`` so a refactor back to QueuePool / SingletonThreadPool can't silently re-introduce the cross- event-loop crashes. - ``test_postgres_engine_ignores_pool_sizing_settings`` — setting DATABASE_POOL_SIZE / DATABASE_MAX_OVERFLOW to huge values must not change pool type (proves the deprecated fields are wired-up no-ops). - ``test_postgres_engine_missing_asyncpg_driver_message`` — guards the existing actionable-error branch when the optional ``[postgres]`` extra isn't installed. Verified: - ``uv run pytest tests/unit/`` — 1028 passed - ``uv run ruff check`` clean on the touched python files Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
51419329b0 |
fix(storage): address PR #798 round-3 review (SonarQube + pool sizing + RETURNING test)
Round-3 fixes. Two threads: - 9 OPEN SonarQube issues caused the "E Security Rating on New Code" gate failure. The bot's diagnosis (sa.text(text_sql) → SQL injection) was a wrong guess; the actual SQ rules firing were different. - Bot's substantive concerns: pool defaults too aggressive, delete_browser_session RETURNING path untested on Postgres, schema_version legacy table created on Postgres, stale module docstring. - User's underlying question on the pool: "isn't 1 connection enough?" Right-sized to 2+5 and documented the concurrency model in ADR-026 so the rationale is durable. SonarQube quality-gate fixes (clears all 9 OPEN issues) ------------------------------------------------------- - BLOCKER S6418: rename `SECRET` constant in test_storage_logging.py to `SENTINEL_PASSWORD_FRAGMENT` + NOSONAR with rationale. - CRITICAL S3776: extract `_build_postgres_engine()` from `initialize()` (was complexity 26 > 15); incidentally creates a clean unit-test seam for engine args. - CRITICAL S4423: `ssl.create_default_context(cafile=...)` is flagged as "weak protocol" — Python 3.10+ already negotiates the strongest available protocol. Explicitly pass `purpose=ssl.Purpose.SERVER_AUTH` and NOSONAR with the Python-version rationale. - MAJOR S3358: split the TLS-mode nested ternary in the engine factory into a `_describe_ssl_arg()` helper. - MAJOR S2068 ×3: bind test app-password literals to local vars and put `# NOSONAR S2068` on the same line as the literal (anchoring requirement) instead of on the closing paren. - MINOR S7503 ×2: `# NOSONAR S7503` on `_Cursor.__aenter__/__aexit__` — they MUST be `async` per the context-manager protocol. Pool sizing right-sized (answers "why so many connections?") ------------------------------------------------------------ - `DATABASE_POOL_SIZE` default 10 → **2**. - `DATABASE_MAX_OVERFLOW` default 20 → **5**. - Per-pod max drops from 30 to 7. With 3 replicas, total = 21 connections (was 90) — well under managed-Postgres `max_connections=100`. - New INFO log at startup: `Postgres engine ready: pool_size=N max_overflow=M (per-pod max K connections)`. Surfaces the active sizing without grepping config. - New ADR-026 § "Concurrency model and pool sizing" explains asyncpg's single-flight connection semantics, the MCP workload shape (read-mostly point lookups), why-not-1 (multi-user serialization), and the tune-up/tune-down recipe. - `docs/configuration.md` table updated with new defaults + homelab-vs-prod tuning guidance, linking the ADR. RETURNING path covered on Postgres ---------------------------------- - New `test_browser_session_delete_returning` exercises the `DELETE … RETURNING user_id` path — the only RETURNING clause in the storage layer and the most dialect-sensitive SQL in this PR. Asserts both present-row (returns True, row gone) and absent-row (returns False) branches. Schema portability polish ------------------------- - `alembic 001`: gate `schema_version` table creation on `op.get_bind().dialect.name == "sqlite"`. The table exists purely to match the fingerprint of pre-Alembic SQLite databases; fresh Postgres installs no longer carry the dead legacy table. Misc polish ----------- - Module docstring: "SQLite-based" → "SQL-backed", with a sentence on the DATABASE_URL opt-in and an ADR-026 link. - Comment on `_wrap_row` noting `row._mapping` is the documented RowMapping accessor in SQLAlchemy 2.x despite the underscore. Skipped (rationale in PR reply) ------------------------------- - `_qmark_to_named` SQL-comment handling: docstring already notes the limitation; no `?` in storage SQL comments today. - Module-level `anyio.Lock()`: established precedent confirmed by the bot itself. - `get_audit_logs` `SELECT *`: pre-existing pattern, out of scope. Verification ------------ - `uv run pytest tests/unit/` — 1025 passed. - `TEST_DATABASE_URL=… uv run pytest tests/integration/test_storage_postgres.py -m postgres` — 7 passed. - `ruff check && ruff format --check && ty check` — clean. - Confirmed `schema_version` absent on fresh Postgres, still present on fresh SQLite. Tracked on Astrolabe Cloud POC board, card #99. --- _This PR was generated with the help of AI, and reviewed by a Human_ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
f2b7bf132f |
fix(storage): address PR #798 review feedback (credentials, asyncpg extra, TLS, pool)
Round-2 fixes after the bot review on PR #798 plus two user follow-ups (self-signed Postgres support; asyncpg should be a PyPI extra). Folded into the same PR rather than a follow-up since the work is still unmerged. Security -------- - Mask database credentials in all 5 log call sites (storage.py × 4, migrations.py × 1) via a new `mask_db_password()` helper in config.py. Uses SQLAlchemy's `make_url(...).render_as_string(hide_password=True)` with a regex fallback so the masking path never raises. - New `tests/unit/test_storage_logging.py` asserts a sentinel password never appears in `caplog` during `RefreshTokenStorage.initialize()`. Distribution ------------ - `asyncpg` moved to `[project.optional-dependencies] postgres` so a vanilla `pip install nextcloud-mcp-server` no longer pulls in the ~5 MB C extension. The Docker image runs `uv sync --extra postgres`, so containerized deployments are unchanged. - When `DATABASE_URL=postgresql+asyncpg://...` is set on a venv missing the extra, `RefreshTokenStorage.initialize()` raises a friendly RuntimeError pointing at `[postgres]` rather than the generic ModuleNotFoundError. TLS for the Postgres backend ---------------------------- - New `DATABASE_VERIFY_SSL` + `DATABASE_CA_BUNDLE` env vars mirror the existing `NEXTCLOUD_VERIFY_SSL` / `NEXTCLOUD_CA_BUNDLE` pattern (validators in Settings.__post_init__, `get_database_ssl()` helper alongside `get_nextcloud_ssl_verify()`). `DATABASE_VERIFY_SSL=false` wins over `DATABASE_CA_BUNDLE` for incident-response convenience. - Default is **None** rather than True — keeps PR #798's behavior intact for cluster-internal Postgres that runs without TLS. Operators opt into verify-full or supply a private CA. ADR-026 records the reasoning vs the Nextcloud HTTPS default. - Engine factory in `storage.py` passes `ssl` via `connect_args` only when `get_database_ssl()` returns non-None; otherwise asyncpg's default (`prefer`) applies. - Storage logs which TLS mode is active at INFO (no secret material). Configurable connection pool ---------------------------- - `DATABASE_POOL_SIZE` (default 10) and `DATABASE_MAX_OVERFLOW` (default 20) replace the hardcoded engine values. With many replicas this can blow past managed-Postgres `max_connections=100`; tune down for large fleets. - gte-1 / gte-0 validators in __post_init__ reject 0/negative pool sizes at startup with the offending value in the error. Consistency polish ------------------ - Migration 006: convert raw `op.execute("ALTER TABLE ... ADD COLUMN")` to `op.batch_alter_table(...).add_column(sa.Column("nonce", sa.Text))` for stylistic consistency with the rewritten 001-005. Downgrade now drops the column instead of being a no-op. - `registered_webhooks.created_at` standardized from `sa.Float` to `sa.BigInteger` (all other `*_at` columns); `store_webhook()` casts `time.time()` → `int`. - `is_sqlite_url()` made case-insensitive. Testing ------- - New `tests/integration/test_storage_postgres.py::test_cleanup_expired_roundtrip` exercises `cleanup_expired_tokens`, `cleanup_expired_sessions`, and `cleanup_expired_browser_sessions` — relies on DELETE rowcount, historically dialect-tricky. - `tests/unit/test_ssl_config.py` extended with `TestDatabaseSSLSettings` + `TestGetDatabaseSSL` classes (9 new tests) mirroring the existing Nextcloud SSL tests one-for-one. Docs ---- - `docs/configuration.md` Centralized-Storage section grew the four new env vars + a homelab example with a private CA. - `docs/ADR-026` grew Distribution, TLS, and `alembic/env.py` async-pattern subsections explaining the non-obvious design choices. Helm chart counterpart in cbcoutinho/helm-charts PR #34 (separate commit on `feat/nextcloud-mcp-server-database-url`). Verification ------------ - `uv run pytest tests/unit/` — 1025 passed. - `TEST_DATABASE_URL=... uv run pytest tests/integration/test_storage_postgres.py -m postgres` — 6 passed (including new cleanup test). - `uv run ruff check && uv run ruff format --check && uv run ty check -- nextcloud_mcp_server` — clean. Tracked on Astrolabe Cloud POC board, card #99. --- _This PR was generated with the help of AI, and reviewed by a Human_ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
292cbb3292 |
feat(storage): pluggable database backend via DATABASE_URL (ADR-026)
Adds a `DATABASE_URL` setting that lets `RefreshTokenStorage` run against
any SQLAlchemy async backend, primarily `postgresql+asyncpg://...` for
HA k8s deployments. Default behavior is unchanged: when `DATABASE_URL` is
unset the server falls back to the existing `TOKEN_STORAGE_DB` path /
ephemeral SQLite tempfile.
Why
---
Today every MCP pod needs its own PVC to hold the SQLite file, which
pins the Deployment to one replica and blocks horizontal scaling. With
this change, operators can point all replicas at a shared Postgres
(CNPG, RDS, etc.) and the pods become stateless. Encryption stays in
Python (Fernet); the database only sees ciphertext.
What changed
------------
- `config.get_database_url()` resolves DATABASE_URL → TOKEN_STORAGE_DB →
ephemeral tempfile in that priority order.
- `RefreshTokenStorage` builds a process-shared `AsyncEngine` in
`initialize()`. SQLite gets NullPool; Postgres gets pool_size=10,
max_overflow=20, pool_pre_ping=True. 30 aiosqlite call sites adapted
via a thin `_DBConn` / `_Cursor` / `_Row` / `_ExecuteCtx` shim so
existing method bodies need no churn beyond the connection
context-manager swap.
- 7 `INSERT OR REPLACE` statements rewritten as portable
`INSERT ... ON CONFLICT (...) DO UPDATE` (SQLite ≥ 3.24, Postgres ≥ 9.5).
- `sqlite_master` legacy-detection lookup replaced with SQLAlchemy
inspector so the path works against either backend.
- File-permission hardening + parent-dir creation gated on
`is_sqlite_url(...)` — centralized backends manage their own filesystem.
- Alembic migrations 001/002/003/005 converted from raw `op.execute(SQL)`
to portable `op.create_table()` / `op.create_index()` with SQLAlchemy
types. All timestamp columns are `sa.BigInteger` so Postgres allocates
BIGINT (unix epochs don't fit in INT4). SQLite treats BIGINT as
INTEGER, so existing deployments at revision 006 see no schema drift.
- `migrations.py` + CLI take URLs; `db {upgrade,downgrade,current,history}`
gain `--database-url / -u` alongside the legacy `--database-path / -d`.
`get_current_revision()` uses SQLAlchemy inspector instead of raw
sqlite3, so the CLI works against Postgres too.
- `docker-compose.yml` adds a `postgres-test` service under the
`postgres` profile (pinned `postgres:16-alpine` digest) for
integration testing.
- Unit storage tests parametrized over backends via shared
`tests/fixtures/storage_backend.py` — every test in
`test_app_password_storage.py` and `test_webhook_storage.py` runs
once per backend that is available. Postgres is opted in by
`TEST_DATABASE_URL`.
- New `tests/integration/test_storage_postgres.py` (5 tests, marked
`postgres` + `integration`) covers refresh-token, app-password,
OAuth-session, webhook, and audit-log paths end-to-end on Postgres.
- New `docs/ADR-026-pluggable-database-backend.md` records the decision;
`docs/configuration.md` documents `DATABASE_URL` with examples.
Out of scope
------------
- No SQLite → Postgres data migration tool (clean cutover; tokens reissue
on next login, webhooks re-register on next sync tick).
- This repo does not provision Postgres. The matching helm chart change
lives in cbcoutinho/helm-charts (database.url / existingSecret values).
Verification
------------
- `uv run pytest tests/unit/` — 1012 passed, SQLite path unchanged.
- `docker compose --profile postgres up -d postgres-test`
- `TEST_DATABASE_URL=... uv run pytest tests/integration/test_storage_postgres.py -m postgres -v`
— 5 passed.
- `TEST_DATABASE_URL=... uv run pytest tests/unit/test_app_password_storage.py
tests/unit/test_webhook_storage.py` — 50 passed (25 per backend).
- `uv run ruff check && uv run ruff format --check && uv run ty check -- nextcloud_mcp_server` — clean.
Tracked on Astrolabe Cloud POC board, card #99.
---
_This PR was generated with the help of AI, and reviewed by a Human_
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
||
|
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 |