Surface the text-merge update path's limitation rather than silently no-op:
when contact_data['email'] or ['tel'] arrives as a dict/list on update, log a
warning at the top of _merge_vcard_properties pointing callers at plain str
or create_contact. Existing EMAIL/TEL lines are still preserved unchanged.
Bring nc_contacts_update_contact docstring into parity with create — the
update tool now documents the same keys plus the explicit single-string
limitation for email/tel and the BDAY validation / URL first-only behaviours.
Three new TestMergeVcardProperties cases pin the warning: dict email warns,
list tel warns, plain str email is silent (no false positives).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- _merge_vcard_properties: list-form ORG was passed through
_safe_vcard_value unchanged, emitting a Python repr on the wire. Both
branches now ;-join list components per RFC 6350 §6.6.4 (ORG is
Company;Department;…) before interpolation.
- _wrap_contact_field: a dict whose ``type`` was a bare string used to
hit ``list("WORK")`` and explode into ``["W","O","R","K"]``. Wrap
bare-string types into a single-element list before the list() call.
Regression tests pin both shapes:
- list-org overwrites and add-new produce ``ORG:Acme;Engineering``
- dict email with ``type="WORK"`` (bare str) emits ``EMAIL;TYPE=WORK:``,
not ``EMAIL;TYPE=W,O,R,K:``.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- _merge_vcard_properties no longer silently drops the existing EMAIL /
TEL line when contact_data supplies a dict/list shape: the input is
unhandled by the text merge, so the original line is preserved
instead of being consumed and replaced with nothing.
- Extracted _parse_bday so the update path validates ISO format the
same way create does. Invalid → keep existing BDAY line (or skip on
add-new) rather than writing a malformed one.
- Added _safe_vcard_value to escape newlines per RFC 6350 §3.4 at every
interpolation site in _merge_vcard_properties, blocking value-driven
property injection (e.g. NOTE: containing a literal \n + EMAIL:).
- Removed dead "organization" alias references from _merge_vcard_properties:
unreachable since update_contact normalises before calling.
- New regression tests pin all four behaviours (dict-email preserves
existing line, list-tel ditto, invalid-bday-update preserves original,
invalid-bday-add-new is dropped, newline-in-note no injection).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pulls the remaining review feedback into one commit:
- Remove the double _normalize_contact_data call: the helper now assumes
canonical keys, and create_contact normalises before calling it
(update_contact already did). Docstring states the invariant.
- Drop phone/organization from _SUPPORTED_CONTACT_KEYS; they never reach
the unknown-key check post-normalisation.
- Tighten generics to dict[str, Any] / list[str] across helpers and
ContactsClient signatures.
- Comment both URL-merge sites noting only the first URL is written.
- Log a warning when fn is missing from contact_data.
- Test coverage for _wrap_contact_field dropping value-less dicts and
for the fn-missing warning; _vcard helper now mirrors the real call
chain (normalise → build).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR #719 review raised a claim that these three fields fall through to
the "keep unchanged" catch-all in _merge_vcard_properties. The existing
elif branches for NICKNAME/BDAY/CATEGORIES already prevent that, but
the behaviour wasn't pinned by a test. Add a focused TestMergeVcardProperties
class that calls the merge helper directly and asserts:
- Existing NICKNAME/BDAY/CATEGORIES lines are overwritten by new values.
- When the existing vCard has none of these lines, update adds them.
- A URL update doesn't clobber unrelated ORG/NOTE/TEL properties.
If the primary update path ever regresses for these fields, these tests
will catch it immediately.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Type-annotate _wrap_contact_field signature; drop stale "url" mention
from its docstring (url is handled by the list-coercion helper, not
this one).
- Split the shape-coercion helper so comma-splitting only applies to
categories: _as_str_list (no split) for org/nickname/url,
_split_categories (comma split) for CATEGORIES. Fixes the case where
organization="Smith, Jones & Associates" was mangled into a two-
component ORG.
- Share _normalize_contact_data between create and update so
_merge_vcard_properties only sees canonical keys; add URL handlers in
both update branches so the primary update path no longer drops URL
silently.
- Annotate the Contact(**kwargs) type:ignore with the reason
(pythonvCard4 typeshed doesn't accept **dict[str, Any]).
- Add tests/unit/client/test_contacts.py (pure unit, no HTTP) covering
the #716 round-trip, comma-in-org regression, invalid-bday warning,
tel/phone precedence, categories string-vs-list behaviour, and direct
_normalize_contact_data cases.
- Extend the MCP workflow test with an update-with-url step asserting
the URL handler in _merge_vcard_properties.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
create_contact previously read only fn/email/tel from contact_data and
silently dropped org, organization, note, title, nickname, bday,
categories, url — and didn't accept phone as an alias for tel, so the
reporter's exact call lost every field except fn and email. Introduce
_build_contact_from_data, share it with update_contact's fallback, and
normalise str→list inputs so pythonvCard4 doesn't iterate bare strings
character-by-character for list-typed properties.
Closes#716
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the per-user delay pattern used in tests/conftest.py:all_oauth_tokens
(commit 963a504). Without it, all four Playwright browser contexts hit
Nextcloud's OIDC authorize endpoint simultaneously and the last users in
iteration order (charlie/diana) frequently time out on the consent screen
in CI, producing `TimeoutError: Timeout waiting for OAuth callback`.
Uses a 0.5s stagger locally and 10s in GITHUB_ACTIONS, matching the
existing fixture so behaviour stays consistent across the two parallel
fixtures.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous run on nc32 failed at the search-result assertion because
`wait_for_vector_sync` returned on the first indexed-count bump (deck
seed cards) before this specific note hit Qdrant. Replace the single
search call with a poll that retries every 2s until the unique term
returns our note, or times out after 60s with a loud diagnostic. The
previously-observed flake would now wait past the deck-card indexing
window rather than racing it.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per review:
- Hoist `import httpx` out of the two test function bodies and into
the module imports at the top of
test_astrolabe_chunk_context.py.
- Simplify the regression guard in
test_management_chunk_context_endpoint.py to use
`mock.assert_awaited_once_with(...)` instead of manually unpacking
call_args. This is stricter — it fails loudly on signature change —
and matches the canonical pattern for asserting mock calls.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Rename test_chunk_context_endpoint_handles_missing_app_password to
test_chunk_context_endpoint_rejects_invalid_bearer so it reflects
what is actually exercised: an invalid bearer is rejected upfront at
validate_token_and_get_user, not at the NotProvisionedError branch.
The NotProvisionedError path is covered by the corresponding unit
test in test_management_chunk_context_endpoint.py.
- Hoist `import base64` to module level per PEP 8.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Astrolabe's ApiController endpoints (search, chunk-context) require a
CSRF `requesttoken` header — axios picks it up from OC.requestToken
automatically in the SPA, but page.request.get() does not.
The first CI run failed on the search step with 412 CSRF check failed
before reaching the chunk-context assertion that was supposed to
surface the handler bug. Load the Astrolabe page, read OC.requestToken,
and pass it on both calls.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The /api/v1/chunk-context and /api/v1/pdf-preview handlers in
api/visualization.py forwarded the incoming OAuth bearer directly to
Nextcloud via NextcloudClient.from_token. In multi-user BasicAuth mode
Nextcloud has no validator for those bearers on Notes/WebDAV, so it
treats the request as anonymous and returns 401 — surfaced to the user
as a 500 from /apps/astrolabe/api/chunk-context. Search worked because
it only hits Qdrant.
Architecturally, OAuth is only for Astrolabe→MCP server; MCP server→
Nextcloud always uses the per-user app password stored during provision
(background sync already does this via vector.oauth_sync).
- Resolve the Nextcloud client through get_user_client_basic_auth in
both get_chunk_context and get_pdf_preview, surfacing
NotProvisionedError as a clean 401 instead of opaque 500.
- Apply the same fix to the session-cookie variant in
auth/viz_routes.chunk_context_endpoint for the internal viz UI.
Tests:
- New unit file test_management_chunk_context_endpoint.py, including a
regression guard that asserts get_user_client_basic_auth is awaited
(so reverting to from_token fails without needing a live Nextcloud).
- Updated test_management_pdf_preview_endpoint.py to mock the new auth
path (drops extract_bearer_token / NextcloudClient.from_token patches).
- New integration test test_astrolabe_chunk_context.py drives the full
chain (browser → Astrolabe → MCP → Nextcloud) in multi-user BasicAuth
mode, plus bare-bones 401 checks on the MCP endpoint.
Full unit suite: 546 passed.
Companion PR on astrolabe (cbcoutinho/astrolabe#66) sends the Nextcloud
UID as loginName in the app-password POST body so the stored record is
complete. Submodule bump to that branch will follow once CI reproduces
the failure on the old submodule.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Picks up astrolabe/astrolabe#61 which fixes app password provisioning
failure caused by loginName mismatch in ITokenProvider::generateToken().
This was the root cause of vector sync never indexing in multi-user
BasicAuth mode, which caused the plotly visualization test to fail.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The multi-user-basic integration job was consistently failing with
`CancelledError: Cancelled via cancel scope ... by <async_generator_athrow>`
followed by a cascade of `anyio.ClosedResourceError` in every subsequent
test. Root cause: `create_mcp_client_session` was declared as an async
generator driven by `async for session in ...:`, so Python's generator
finalizer (`aclose`) ran under pytest-asyncio's cleanup task instead of
the task that owned the nested `streamablehttp_client` cancel scope.
anyio then raised when the inner task group saw its scope being exited
from a foreign task, leaving the memory object streams half-closed and
poisoning the rest of the session.
Switching to `@asynccontextmanager` + `async with ... as session:` makes
`__aenter__`/`__aexit__` run in the frame that owns the context manager,
satisfying anyio's structured concurrency requirements.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- _resolve_settings_files() now raises FileNotFoundError when
NEXTCLOUD_MCP_SETTINGS_FILE points to a missing file, instead of
silently falling back to defaults (footgun on typos).
- .secrets.toml is now looked for alongside the explicit settings file
when NEXTCLOUD_MCP_SETTINGS_FILE is set, matching user expectation for
/etc-style deployments. Unset behaviour (cwd lookup) is unchanged.
- get_token_db_path() drops the redundant os.environ.get() short-circuit;
TOKEN_STORAGE_DB is already bound through dynaconf because the key is
declared in _DEFAULTS.
- is_ephemeral_token_db() docstring documents the "must call
get_token_db_path() first" precondition.
- alembic.ini comment clarifies the ./tokens.db placeholder is cwd-relative
by design and points readers at the -x database_url escape hatch.
- New tests/unit/test_config_paths.py (12 tests) covering the ephemeral
tempfile lifecycle, the TOKEN_STORAGE_DB override path, and all six
_resolve_settings_files() cases including the two new behaviours.
Full unit suite now at 476 passed (464 + 12 new). Ruff + ty clean.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two bugs made `uvx --from . nextcloud-mcp-server run` (and any pip install)
unusable outside Docker:
1. Dynaconf was configured with ignore_unknown_envvars=True and relied on
settings.toml to declare the key schema. With no settings.toml in a wheel
install, every env var (NEXTCLOUD_HOST, MCP_DEPLOYMENT_MODE, ...) was
silently dropped. Moved the schema into a Python _DEFAULTS dict passed
directly to Dynaconf, kept settings.toml as an optional external override
(renamed to settings.toml.example, gitignored), and pointed docker-compose
at the example file.
2. Token SQLite DB defaulted to /app/data/tokens.db in multiple places
(auth/storage.py, migrations.py, alembic/env.py, cli.py db subcommands),
which blew up at uvicorn startup with FileNotFoundError on non-Docker
hosts. Replaced with a new config.get_token_db_path() helper that
resolves TOKEN_STORAGE_DB if explicitly set, otherwise allocates a
per-process tempfile cleaned up at interpreter exit via atexit — mirroring
the "ephemeral by default" pattern used for QDRANT_LOCATION=:memory:.
Containers are unaffected: docker-compose services now explicitly set
TOKEN_STORAGE_DB=/app/data/tokens.db (the fourth service that was missing
this pin has been brought in line with the other three).
Verified end-to-end in an isolated /tmp venv: env-var-only startup, Alembic
migrations run against the tempfile, Application startup complete, /health/live
returns 200, tempfile deleted on SIGTERM. Unit tests (464) + ruff + ty pass.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
External IdPs like AWS Cognito return scopes prefixed with the resource
server identifier (e.g. https://mcp.example.com/notes.read). MCP tools
use bare scope names (notes.read) in @require_scopes decorators. Without
stripping the prefix, scope matching fails and only identity-only tools
(openid/profile/email) are visible — resulting in 4/125 tools shown.
Strip the OIDC_RESOURCE_SERVER_ID prefix in both get_access_token_scopes()
(used by list_tools filtering) and the require_scopes decorator (used at
tool execution time).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add BasicAuthLifespanContext Protocol to make the contract between
StdioContext and get_client() explicit and type-safe. Document why
mcp.get_context() is required for non-template resources. Add News
and Collectives to README Supported Apps table, fix transport default.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
AWS Cognito provides refresh tokens automatically with the authorization
code flow but does not list offline_access as a supported scope. Check
the IdP's scopes_supported discovery field before including it in
requests, and always accept refresh tokens from responses regardless.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove dead monkeypatch in test_stdio_calls_get_stdio_mcp
- Add _reload_config() teardown to single_user_env fixture
- Tighten AVAILABLE_APPS type to Callable[[FastMCP], None]
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
