Round-3 review on PR #891 (no blockers):
- Add test_encode_dav_path_encodes_exactly_once pinning the documented
decoded-input precondition ("already%20encoded.pdf" -> "already%2520...").
- format_exception_group: proper singular/plural ("1 sub-exception" vs
"N sub-exceptions") instead of "(s)".
- oauth_sync: use `if doc_task is not None:` to match processor_task's guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-3 review on PR #892 found a real bug: the gateway backend's httpx.Timeout
raises httpx.ReadTimeout (a httpx.TimeoutException, NOT a builtin TimeoutError),
so the `except TimeoutError` added in r2 only covered the Mistral
(anyio.fail_after) path — gateway timeouts still fell through to
reason="error". Catch both (TimeoutError, httpx.TimeoutException) so either
backend's timeout lands in the dedicated parse_failed_reason="timeout" bucket.
Add an end-to-end test driving a gateway httpx.ReadTimeout through the
processor.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 review on PR #891 (non-blocking):
- Add a parametrised test_webdav_path_encoding covering empty path,
leading-slash stripping, '#'/comma/space, and a non-ASCII name — the single
source of truth for every caller-path builder's encoding, so write_file /
delete_resource / create_directory / attachments are covered transitively.
- Document the decoded-input precondition on _webdav_path (encode-exactly-once;
passing an already-encoded path would double-encode).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 review on PR #892:
- OcrProcessor.process now catches TimeoutError separately and returns
parse_failed_reason="timeout" with a populated message ("OCR timed out after
Ns"), instead of conflating timeouts with API errors under "error" and logging
an empty suffix. Lets dashboards tell a too-low timeout from a failing
provider. Test added.
- Add validator-rejection tests for DOCUMENT_OCR_TIMEOUT_SECONDS=0 (gte=1) and
DOCUMENT_MAX_PDF_SIZE_MB=-1 (gte=0), matching the existing validator-test
pattern.
- Comment the _Settings test fixture's max_pdf_size_mb=0.0 default.
SonarCloud: quality gate was failing on new_security_hotspots_reviewed (S5332
"use https") from an http:// URL in the new gateway-timeout test — switched to
https:// (mirrors commit 98c9d58e).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Collapse _init_worker_observability's docstring to one line; the WHY moves
to a concise inline comment (per review).
- Note that _fake_settings.ingest_queue is unused by the helper (test realism).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- tests: use https in the OTLP endpoint fixture to clear the S5332
"http protocol is insecure" security hotspot (quality gate:
new_security_hotspots_reviewed).
- cli: add the "tracing disabled" else branch in
_init_worker_observability so the worker logs parity with app.py when no
OTLP endpoint is set.
- cli: trim the verbose inline comment in worker() (the WHY lives in the
helper docstring), per review.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review on PR #892:
- Wire DOCUMENT_OCR_TIMEOUT_SECONDS into _MistralOcrBackend too (was
gateway-only): wrap process_async in anyio.fail_after so the SDK-managed
client honours the setting; on expiry it fails fast as a clean parse error.
Test added.
- Tighten the misleading "honoured without a restart" comment — per-call
get_settings() is for test monkeypatching; a live change still needs a
restart since the backend is cached for the pod lifetime.
- Comment the size guard's two intentional gaps: an explicit processor_name
override bypasses it, and the early return skips the parse-duration histogram.
SonarCloud (new-code smells in the added tests):
- S1244 float-equality asserts → pytest.approx (test_config.py, test_ocr_processor.py).
- S1186/S7503: rewrite the gateway-timeout test with mocker AsyncMock/MagicMock
instead of a hand-rolled fake client (no empty method, no async-without-await).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review on PR #891:
- Guard processor_task's broad except handler against an unbound doc_task
(mirrors multi_user_processor_task): initialise doc_task=None before the loop
and branch the error log. Fixes a latent NameError if receive() raises a
non-TimeoutError/EndOfStream before the first document binds. Regression test
added.
- Drop the unnecessary `from __future__ import annotations` in vector/_errors.py
and express format_exception_group's non-group fast path as an explicit
isinstance check.
- Add a copy_resource Destination-header encoding test (analogue to MOVE);
strengthen the ExceptionGroup test to assert the full leaf repr survives.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The external split-worker ingest pods (MCP_ROLE=worker / procrastinate) had
no observability: the worker CLI entrypoint never started a Prometheus
metrics server and never configured structured logging, so the pods that do
the real parse/embed/upsert work were invisible to Prometheus and emitted
plain-text logs the platform pipeline couldn't parse.
The always-on API pod bootstraps observability in its lifespan (app.py), but
the worker has its own entrypoint and never went through that path (or
uvicorn's JSON log_config). Add `_init_worker_observability()` mirroring the
API pod: setup_logging (JSON), setup_metrics on METRICS_PORT when
METRICS_ENABLED, and setup_tracing when an OTLP endpoint is configured.
Runs after the INGEST_QUEUE=postgres check so a misconfigured worker fails
fast without binding a metrics port.
This also unblocks the document-pipeline observability shipped in #831
(Deck #175): the astrolabe_* parse/embed/chunk metrics and the
document_processor.parse span are recorded in the shared registry/processor
code the worker executes — they were simply never exposed in external mode
because the worker served no /metrics and set up no tracer.
Deck #310, unblocks #175.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two ingest-robustness fixes from card 309 (OHR-Bench smoke-test triage).
The OCR backend timeout was a hardcoded 180s module constant, so a tenant
whose gateway has its own shorter ceiling couldn't tune it. Promote it to
DOCUMENT_OCR_TIMEOUT_SECONDS (default 180), resolved per call via get_settings
so an override applies without a restart.
Large, awkward PDFs (e.g. a 42 MB scanned DUDE) were handed straight to the
fast/OCR tiers, where they burned the full OCR timeout for zero recovered
text. Add a pre-parse size guard in the tiered PDF pipeline: a PDF over
DOCUMENT_MAX_PDF_SIZE_MB (default 50, 0 disables) fails fast with
parse_failed_reason="oversize" before any tier runs, so the existing
permanent-failure path marks the placeholder failed and records
astrolabe_document_parse_failed_total{reason="oversize"} instead of retrying.
Both knobs go through Settings + dynaconf validators (env-var keys verified by
regression tests) and are documented under Background Indexing Configuration.
Refs: Deck board 12 card 309 (AC #3 OCR timeout + size guard).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two ingest-robustness fixes from card 309 (OHR-Bench smoke-test triage).
WebDAV paths flowed through the client already URL-decoded (unquote on the
PROPFIND/REPORT <d:href>, or raw MCP-tool input), so a '#' reached httpx as a
URL fragment and silently truncated the request -> spurious 404 on otherwise
valid files (e.g. law filenames with '#', commas, double/trailing spaces).
Route every caller-path builder through a new _webdav_path helper that
percent-encodes the path once (preserving separators); the MOVE/COPY
Destination header is encoded too.
Vector-sync runs inside anyio task groups, so a child-task failure surfaced as
a BaseExceptionGroup whose str() is the useless "unhandled errors in a
TaskGroup (N sub-exception)" -- hiding the real ConnectError operators need.
Add format_exception_group to flatten the group to its leaf exceptions and use
it at the broad catch/log sites in processor.py and oauth_sync.py.
Refs: Deck board 12 card 309 (AC #4 filename handling, AC #2 observability).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-6 review: the docstrings listed preserved fields but omitted
assignedUsers. Verified empirically (Deck 1.15.9) that the update route's
board-change handling only remaps labels and leaves user assignments
untouched, so assignees carry over. Documented in both the client and MCP
tool docstrings, with the caveat that an assignee lacking access to the
target board stays assigned but cannot act on the card. Added
test_move_card_to_board_preserves_assigned_users to lock it in.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The move-card unit tests added a mock httpx.Request with an http:// URL,
which SonarCloud flags as a new security hotspot (insecure protocol),
failing the new-code quality gate. The URL is never dialed (it only labels
a synthetic HTTPStatusError), but switch it to https to keep the gate green.
Also simplify the done-PUT mock to a bare 200 response, since that response
is discarded by the implementation.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-4 review polish on PR #885:
- The post-move done re-mark is now best-effort: the move PUT has already
committed by then, so if the /done call (or its re-fetch) fails, log a
warning with the card's new location and return the moved card instead of
raising as if the whole move failed. Documented in the docstring.
- Note that duedate is sent explicitly as None (vs update_card omitting it) —
equivalent for this route.
- Add unit coverage for the swallowed done-restore failure, and an integration
test for a card that is both done and archived (exercises the done-restore
re-fetch on an archived card).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-3 review polish on PR #885:
- deck_move_card_to_board now captures the moved DeckCard and returns its
post-move label titles in CardOperationResponse.labels, so LLM clients can
confirm the cross-board label remap (the tool's headline behaviour) without
a follow-up deck_get_card. The field is optional and defaults to None for
the other card operations that share this response model.
- Tighten test_move_card_to_board_restores_done_state to assert the returned
card reflects the restored done state.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 review polish on PR #885:
- Document in the deck_move_card_to_board tool that the move reassigns the
card owner to the calling user and resets the done timestamp (both are
limitations of Deck's move route), so an LLM reading only the tool
description isn't misled about preserved fields.
- Fix the done integration-test docstring to say "done state (not timestamp)".
- Add test_move_card_to_board_preserves_archived_status to lock in the
documented archived-preservation behaviour.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
