Addresses the round-1 review on PR #885:
- Preserve `done` across a cross-board move. The internal card-update route
(the only one that works cross-board — the board/stack-scoped route 404s for
a card not already on that board) does not accept a done value, so a "done"
card is re-marked done after the move. Deck stamps the current time there, so
the original timestamp isn't preserved — documented as a route limitation.
(`archived` is already preserved: CardService only mutates it when sent.)
- Validate that target_stack_id is on target_board_id before moving, so the
parameter is load-bearing and a mismatch fails loudly instead of misreporting.
- Skip the same-board guard's get_stacks round-trip on a same-stack reorder.
- Add unit coverage (done-restore call, destination validation, same-stack
skip) and integration coverage (done preservation, target-board mismatch).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
deck_reorder_card only relocated a card between stacks on the same board.
Moving a card to another board now has a dedicated tool that goes through
Deck's card-update route (CardService::update), which remaps the card's
board-scoped labels to the destination board by title instead of leaving
orphaned labels behind. Card identity (id, comments, attachments) is
preserved.
reorder_card is now restricted to same-board moves: it rejects a
target_stack_id on another board (which Deck's reorder route would accept
but with orphaned labels), steering clients to deck_move_card_to_board.
Verified empirically against Deck 1.15.9: the reorder route leaves a moved
card carrying its source board's label (boardId mismatch); the update route
remaps it to the destination board's same-titled label.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- _default_mcp_server_url() replaces the hardcoded localhost:8000 fallback,
deriving the port from settings.port so a custom PORT is honoured and all
fallback sites share one source of truth (removes the footgun where
settings.port looked wired but the OAuth-audience fallback ignored it).
- Clear _readiness_cache.statuses at loop start so dependency entries from a
prior lifespan run in the same process (integration matrix) don't linger as
stale, confusing checks output.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the blanket tg.cancel_scope.cancel() with a per-task CancelScope: the
readiness loop reports its scope via task_status, and the lifespan cancels just
that scope at shutdown. The task group's exit then waits for the scanner/
processor tasks to drain on their shutdown_event instead of force-cancelling
them mid-work, restoring the graceful drain the pre-refactor code had.
Also use docstrings instead of bare `return` in the no-op mode closures.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_readiness_refresh_loop is started with tg.start_soon and loops forever with no
shutdown_event check. anyio waits for start_soon tasks on normal task-group exit
rather than cancelling them, so graceful shutdown hung until uvicorn's timeout.
Cancel the task group's scope after teardown() to stop the loop and any
stragglers, mirroring _maybe_login_flow_cleanup.
Also document the cache ttl_seconds startup-override and the inclusive is_stale
boundary.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- _DEFAULTS keys for NEXTCLOUD_OIDC_TOKEN_TYPE / NEXTCLOUD_OIDC_SCOPES were
registered as oidc_* (uppercasing to OIDC_*), so dynaconf
(ignore_unknown_envvars) never read the NEXTCLOUD_-prefixed env vars and the
fields stayed at their defaults. Prefix the keys to match _field_map; add a
regression test.
- Add gte=1 validator for HEALTH_READY_REFRESH_INTERVAL and a 1..65535 range
validator for PORT.
- Tie ReadinessCache.ttl_seconds to 2x the configured refresh interval so
is_stale() stays meaningful when the interval is tuned.
- Raise the refresh-loop exception log from DEBUG to WARNING.
- Make health_ready a sync handler (no awaits); dedupe the localhost fallback
into _DEFAULT_MCP_SERVER_URL; use pytest.approx for the float default.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-4 review (real bug): get_background_sync_status now returns provisioned_at
as Unix seconds (the wire/pact value), but ProvisioningStatus.provisioned_at is
str | None (ISO). Constructing it for a provisioned user raised a Pydantic
ValidationError — a path that was unreachable before the has_access fix.
Convert int -> ISO at the oauth_tools boundary (mirroring the existing
refresh_token branch), keeping the model schema and the int-asserting contract
pact/unit tests intact. Add a regression test that drives the full
_get_provisioning_status round-trip with an integer timestamp.
Also surface dropped provider-state params in the verifier's _dispatch_state
no-op branch (round-4 nit).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes MCP reconnect timeouts on tenant servers (Deck #302). Three changes:
- /health/ready now gates only on local config. Nextcloud/Qdrant health is
refreshed by a background loop, cached, and reported but NON-gating, so a
single-replica tenant Pod is no longer pulled from its Service on a transient
dependency blip (which dropped every MCP streamable-HTTP session and caused
reconnect timeouts). The probe path performs no external I/O.
- Refactor starlette_lifespan: collapse the four near-identical per-mode
task-group + session + yield + teardown skeletons into one shared task group
that also runs the readiness refresh loop; each mode contributes a
(start, teardown) pair. eviction_task_group is now always present.
- Migrate app.py off os.getenv: all config is read through dynaconf Settings
(adds health_ready_refresh_interval, oidc_token_type, oidc_scopes, port).
Inline/dynamic defaults preserved at each call site.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `env` context is not available in a job-level `if:` (only `github`/`needs`/
`vars`/`inputs` are), so `if: ... && env.PACT_BROKER != ''` on the job was an
invalid-context error that failed the whole workflow to parse. Move the broker
guard onto each step (matching the consumer/provider jobs) and keep the job
`if` on the master-branch check only.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- pact.yml: guard `can-i-deploy` job on `env.PACT_BROKER != ''` so a secret
rotation/fork can't break every master merge (the CLI errors on empty URL)
- pact.yml: pin install.sh to the v2.6.1 commit SHA (immune to tag force-push)
- astrolabe_client.py: `_token_cache` Optional[dict] -> `dict | None` and drop
the now-unused `Optional` import (CLAUDE.md union syntax)
- add tests/unit/test_astrolabe_client.py: mocked unit coverage for
get_background_sync_status field mapping (200 provisioned / 200 not-provisioned
/ 404) — the layer that would have caught the original silent app_password bug
- consumer pact test: note the 404 branch is internal defensive handling (covered
by the unit test), not a contract obligation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- pact.yml: pin tailscale/github-action@v3 to commit SHA (3 jobs) and
pact-ruby-standalone install.sh to v2.6.1 (2 jobs) — supply-chain hardening
- pact.yml: drop redundant `-o "addopts=..."` override (pyproject.toml already
sets the same addopts; the override would silently mask future additions)
- test_mcp_provider_verification.py: remove dead `pytestmark` shadowed by the
list assignment; gate the module skip on PACT_USERNAME/PACT_PASSWORD too so a
broker-set-but-creds-missing CI skips cleanly instead of raising KeyError
- conftest.py: drop the unused `pact_dir` fixture
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Introduce consumer-driven contract testing between nextcloud-mcp-server and the
astrolabe Nextcloud app, published to the homelab Pact Broker and verified in CI.
- pact-python dev dep + `contract` pytest marker
- tests/contract/test_astrolabe_credentials_consumer.py: consumer pact for the
background-sync *status* call (provisioned -> has_background_access:true,
sync_type:"app_password", integer provisioned_at; unprovisioned -> false/null)
- tests/contract/test_mcp_provider_verification.py: env-gated Verifier harness
for this server's /api/v1/* provider role (provider-state handlers stubbed
pending astrolabe's published pacts)
- .github/workflows/pact.yml: join tailnet -> publish pacts -> provider verify
-> can-i-deploy; broker steps skip when PACT_BROKER is unset (forks)
- docs/ADR-029-pact-contract-testing.md
Fix astrolabe_client.get_background_sync_status: it previously read a
non-existent `app_password` field (always reporting no-access). Rewrite it to
read the real status contract (has_background_access / sync_type /
provisioned_at) and drop the unsatisfiable get_user_app_password.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round-2 review nit (PR #879): lock the intentional record ordering
(tokens_embedded before the conditional pages_embedded) with an
assertion in test_parsed_file_records_pages_and_tokens, so a refactor
that reverses it fails a test rather than only contradicting a comment.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review follow-ups (PR #879):
- Gate pages_embedded on `page_count and page_count > 0` so a malformed
negative count meters as "no pages" rather than emitting a negative
billing row (matches the documented call-site intent).
- Exclude bool at the call-site narrowing (`isinstance(int) and not
isinstance(bool)`) — bool is an int subclass, so a stray page_count=True
would otherwise record pages=1.
- Document chunk_count's role (empty-batch no-op guard) and the
intentional tokens-before-pages ordering in the docstring/comments.
- Add test_negative_pages_skips_pages.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`pages_embedded` carried an interim chunk count (`len(chunk_texts)`,
TODO #282). Reframe it as a charge for *parsing* (PDF page extraction /
OCR) rather than a normalized content size:
- Parsed files (PDFs) record `pages_embedded` = real `page_count` from
the document processor metadata.
- Text content (notes, deck cards, news items) is never parsed, carries
no `page_count`, and records no `pages_embedded` row — only
`tokens_embedded`. There is deliberately no chars/tokens-per-page
constant; pages map 1:1 to parsed document pages.
`record_indexing_usage` now takes `page_count` and records the two
dimensions independently, gating `pages_embedded` on a truthy page count
(not the doc_type) so a future non-PDF parsed type stays correct. Stays
flag-gated + best-effort. Tests cover parsed-file, text-only, and
zero-page cases.
Deck #282 (board 8). Billing-model ADR corrected in
astrolabe-cloud-website docs/control-plane/usage-metering.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses round-1 review on #878:
- Move the eager `document_processors` imports out of the API startup graph:
`app.py` (get_registry now imported inside initialize_document_processors,
after the disabled early-return) and `vector/processor.py` (get_registry now
imported at its single use site). Importing `app` + `cli` no longer loads
`document_processors` / `_isolation` at all -- the #877 stack is fully out of
startup (pymupdf still loads via search/pdf_highlighter, a Windows-compatible
and separately-tracked concern).
- Make `tests/unit/test_pdf_parse_isolation.py` importable on Windows: guard the
top-level `import resource` with try/except and skip the three rlimit
computation tests via a `requires_resource` marker when the module is absent.
The Windows no-op / import-guard tests don't use the real module and still run.
- Fix the `# pragma: no cover` comment on the win32 branch to be accurate.
- Add `enable-cache: true` to the package-smoke setup-uv step.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`document_processors/_isolation.py` did an unconditional module-level
`import resource`, a POSIX-only stdlib module absent on Windows. It was
pulled into the API startup path via
`server/webdav.py -> utils/document_parser -> document_processors`, so
the MCP server failed to start on Windows since 0.101.2 with
`ModuleNotFoundError: No module named 'resource'`.
- Guard the import behind `sys.platform`; bind `resource = None` on
win32. `_apply_mem_limit()` degrades to a logged no-op when the module
is unavailable (the RLIMIT_AS cap is a Linux-pod safety measure, not a
correctness requirement).
- Make the document-parser import in `server/webdav.py` lazy so server
startup never loads the ingest document stack
(document_processors -> pymupdf -> _isolation) at all -- it is only
needed when a file is actually read and parsed. This both fixes#877
and decouples the API layer from ingest-only deps.
- Add unit regressions for the no-op path and the win32 import guard.
- Add a cross-platform `package-smoke` CI job (ubuntu + windows) that
installs the package isolated and runs the CLI, exercising the
cli -> server -> webdav import chain that crashed in #877.
Fixes#877
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-8 claude-review (no blockers; comment-only):
- 🟡 Documented that Ollama's /api/embed prompt_eval_count is assumed
batch-level total and is unverified against a live instance (Ollama isn't the
Cloud billing provider); if it proves last-item-only, switch to per-item
summing. The char estimate already covers versions that omit the field.
- 🟡 Noted on the astrolabe_embedding_tokens_total counter that operation="query"
is recorded pre-Qdrant, so it can legitimately exceed the billing-store
tokens_embedded aggregate when a search fails post-embed — dashboards
shouldn't alert on that healthy gap.
Deferred (reviewer: "minor nit, acceptable"): record_indexing_usage awaited in
the task group — the group awaits all child tasks regardless, the write is
best-effort + fast, and start_soon would need the tg threaded into the closure
for marginal gain.
Deck #284.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-7 claude-review (no blockers):
- 🟡 query_token_count/query_embedding were class-level defaults on
SearchAlgorithm, relying on each subclass's __init__ to shadow them. Added
SearchAlgorithm.__init__ that sets both as instance attributes and had
BM25HybridSearchAlgorithm + SemanticSearchAlgorithm call super().__init__(),
so per-request concurrency isolation is structural, not by convention.
- 🟡 Documented the v1 search-path billing gap: record_search_usage fires only
on a fully successful search, so if the query embed succeeded (provider billed
+ Prometheus recorded) but a later step (Qdrant/verify) raised, no
tokens_embedded billing row is written. Added a NOTE at the call site.
Left as-is (reasons in PR reply): deployment sequencing (CP METRIC_EVENT_NAMES
already renamed; pipeline inert); Ollama _detect_dimension double dimension-set
(idempotent, same value); SonarQube issues — 1 is the deliberate TODO(#282)
(INFO), 4 are S7503 false positives on async test stubs that must be awaitable
(gate green).
Deck #284.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Billing product model finalized (Deck #281): bill pages externally, record
tokens internally. Rename the data-plane metric literals to match the now-
canonical contract (Deck #284) — the control plane's METRIC_EVENT_NAMES is
already renamed, so the old names would be unmapped and never sync to Stripe.
Rename (values unchanged):
- embeddings_queries → tokens_embedded (value = real token count, already
emitted by this PR; the unit upstream providers bill on).
- pages_chunks → pages_embedded (value kept as len(chunk_texts) interim;
TODO(#282): real normalized "pages indexed" count — real pages for paginated
types, chars/tokens-per-page constant otherwise — is deferred to the
instrumentation card, this only lands the name/contract).
- All literals, log strings, docstrings, comments, the migration comment, and
tests renamed; grep confirms zero old strings remain.
Observability (new): export embedding token cost to Prometheus as
astrolabe_embedding_tokens_total{provider,operation} (operation = index|query)
so the billed cost unit is visible in Grafana, not just the per-tenant billing
DB. Dedicated counter (doesn't inflate the existing chunk/request metrics) and
always-on (independent of USAGE_METERING_ENABLED, so OSS/self-host gets it).
Wired on both the indexing batch embed and the search query embed (query inside
the per-request cache-miss branch, so reused embeddings aren't double-counted).
Note: the rename orphans any pre-existing embeddings_queries/pages_chunks rows
in tenant app DBs (CP no longer maps them) — acceptable; pipeline is inert with
throwaway dev/sandbox data.
Deck #284 (folded into PR #875).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-6 claude-review (ready to merge): the three new GatewayProvider
usage/bearer tests lacked @pytest.mark.unit, so `pytest -m unit` skipped them
even though every other new test in this PR is marked. Add the marker to the
three new tests (leaving the pre-existing unmarked tests in the file alone).
Remaining 🟢 items (OpenAI embed() dual path, recursion-invariant runtime
enforcement, Bedrock sync-in-async) are acknowledged deferrals — separate
refactors, unchanged.
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-5 claude-review (merge-ready; all nits):
- 🟡 Added test_empty_doc_types_normalizes_to_null pinning doc_types=[] → None
in record_search_usage metadata (matches the None case).
- 🟡 record_search_usage docstring now notes nc_semantic_search_answer always
meters with doc_types=None (it exposes no doc_types parameter).
- 🟢 BM25HybridSearchAlgorithm.__init__ now sets query_embedding /
query_token_count alongside _embedded_query, so all three cache fields are
instance attributes from construction (was relying on the class-level
SearchAlgorithm defaults).
- 🟢 Ollama embed_batch_with_usage caches _dimension inline (mirrors
OpenAI/Mistral), so the dimension is set via any embed path.
- 🟢 record_indexing_usage documents the independent-record / partial-failure
semantics under SUM aggregation.
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Drop the redundant `.rstrip("/")` in `_list_object_names`; the
`endswith("/")` guard already excludes the collection entry.
- Remove the now-unused `_get_raw_vcard` (update_contact resolves the name
itself and calls `_fetch_raw_vcard` directly). Its only remaining caller —
the create→read integration test — now calls `_fetch_raw_vcard` with the
deterministic `<uid>.vcf`, saving a redundant PROPFIND.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Use str.removesuffix(".vcf") instead of str.replace(".vcf", "") in both
_resolve_object_name and list_contacts so a filename like "alice.vcf.backup"
isn't mangled; the two transforms stay consistent to preserve the
surface-then-resolve round-trip.
- Add update_contact resolution tests mirroring the delete coverage:
targets the real no-extension path, and falls back to <uid>.vcf when
resolution finds nothing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-4 claude-review findings (no blockers):
- 🟡 Untested server-layer metering hook (raised across rounds): extracted the
nc_semantic_search embeddings_queries recording into a module-level
record_search_usage() helper (mirroring record_indexing_usage) and added
tests/unit/server/test_semantic_metering.py — value = query token count,
flag-off no-op, None token → 0, doc_types metadata bounding, best-effort
failure swallowed.
- 🟡 Dedup-hit skipped metering invisibly: the existing dedup info log now
states "no embedding/usage recorded" so a "fewer embeddings_queries rows than
expected" audit lands on the dedup path directly.
Deferred 🟢 nits (stated on the PR): search 0-token rows are recorded
deliberately (the query embedding ran; zero is a sum no-op) — documented in the
helper; embed_tokens closure locality and the OpenAI embed() dual path are
unchanged (correct as-is / separate refactor).
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-3 claude-review finding:
- 🟡 Double _ensure_bearer() on gateway.embed_batch(). Round 1 made
OpenAIProvider.embed_batch() delegate to embed_batch_with_usage(); because
GatewayProvider overrode both embed_batch() and embed_batch_with_usage() (each
calling _ensure_bearer), gateway.embed_batch() refreshed the bearer twice
(the second a cache-hit no-op). Remove the now-redundant embed_batch()
override: OpenAI's embed_batch() routes through embed_batch_with_usage(),
which the gateway still overrides, so the bearer refreshes exactly once on
every path. The remaining two overrides (embed + embed_batch_with_usage) cover
all four entrypoints; documented the topology.
- 🟢 Added test_gateway_embed_batch_ensures_bearer_once locking in the single
refresh.
Cohere token-fallback (🟢 nit) is already covered by
test_bedrock_with_usage_estimates_when_token_count_absent.
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 claude-review findings:
- 🟡 Base-class recursion invariant: documented on embed_with_usage /
embed_batch_with_usage that a provider overriding embed()/embed_batch() to
delegate to the *_with_usage variant MUST also override that variant, or the
two recurse. (No recursion today; the shipped providers pair the overrides.)
- 🟡 Processor metering had no unit test: extracted the two-event recording
into a module-level record_indexing_usage() helper and added
tests/unit/test_processor_metering.py (value mapping, flag/zero-chunk no-ops,
best-effort failure swallowed).
- 🟡 SonarQube hotspots (python:S5332) were 3 http:// URLs in the new test
fixtures (mock hosts, never contacted) blocking the quality gate
(new_security_hotspots_reviewed). Switched them to https:// so no hotspot is
raised.
- 🟢 Zero-chunk guard: record_indexing_usage() no-ops when chunk_count == 0, so
an empty document no longer writes zero-value billing rows.
Deferred (stated on the PR): Mistral x.index-or-0 sort key (pre-existing,
equivalent), CHANGELOG note for the Ollama /api/embed switch (CHANGELOG is
commitizen-generated from commit bodies, which document it), class-var
query_token_count (safe under the per-request instance pattern).
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
