Round 3 review (PR #910):
- Guard an unexpected terminal batch status in _process_batch: anything that
isn't succeeded/failed (gateway version skew, a new lifecycle state) now marks
the document parse-failed instead of falling through to _pages_to_text([]) — a
0-chunk "success" that silently indexed empty text and re-submitted forever.
Test added.
- gateway_batch_client.submit: raise an actionable ValueError on a 2xx response
with no job_id (was a bare KeyError deep in the caller).
- Document that a _process_batch transport error intentionally propagates to
procrastinate for retry rather than falling back to sync (opt-in batch wants
the retry).
- Annotate _batch_client as GatewayBatchOcrClient | None (TYPE_CHECKING import
already present); clarify the delete_stale_for_doc first-submit no-op comment.
- Add a parametrized build_gateway_batch_client test (the gateway-only invariant:
mistral/none/no-URL -> None; gateway|auto + URL -> client).
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 2 review (PR #910):
- BLOCKING: BatchOcrJobStore._shared_lock is now lazy-init (anyio.Lock | None,
created on first shared() call) instead of at class-definition time — matches
the CLAUDE.md "no anyio primitives at import time" rule and OcrProcessor's
pattern. The None-check->assign has no await between, so it's race-free.
- document_ocr_mode now normalizes via _enum_fields (case-insensitive, like
document_ocr_provider) instead of a strict dynaconf is_in Validator, so
DOCUMENT_OCR_MODE=Batch normalizes to "batch" rather than erroring. Tests for
case-normalization + invalid-value rejection.
- TYPE_CHECKING-gated GatewayBatchOcrClient import so build_gateway_batch_client
/ _get_batch_client are typed `GatewayBatchOcrClient | None` instead of Any
(runtime import stays lazy to avoid the import cycle).
- Rename ocr_options -> doc_identity_options (it's threaded to all tiers; only
OCR reads it) + clarify the comment.
- Drop the redundant forward-ref quotes on _shared_instance.
- Add direct _batch_identity unit tests (partial/empty options branches).
Left as follow-up: reusing one httpx.AsyncClient across submit/poll (same
per-call pattern as the existing sync _GatewayOcrBackend; no clean aclose hook
on the cached client today).
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 1 review (PR #910):
- BLOCKING: add document_ocr_mode / _batch_poll_seconds / _batch_max_wait_seconds
to config._field_map — without it dynaconf silently ignored the env vars and
DOCUMENT_OCR_MODE=batch could never be enabled in production. Add a regression
test asserting the three round-trip from env.
- migration 008: give batch_ocr_jobs a composite PRIMARY KEY on
(user_id, doc_id, doc_type, etag) instead of a bare UniqueConstraint (N1).
- OcrProcessor: use a dedicated _batch_client_lock instead of sharing the sync
backend lock (N3).
- tests: use https:// gateway URLs in the new fixtures to clear SonarCloud's
"insecure http" security hotspots (all 14 were test-only http://gw literals).
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add DOCUMENT_OCR_MODE=sync|batch (default sync). In batch mode the tier-3 OCR
processor submits documents to the embedding gateway's async Batch OCR routes
(POST /v1/ocr/batch + GET /v1/ocr/batch/{job_id}, astrolabe-cloud-website#372)
for ~50% cheaper large-corpus backfill. The direct Mistral OCR path is left
untouched. Tracked on Deck #332.
Batch jobs run minutes-hours, so the OCR tier cannot block (the procrastinate
worker reclaims jobs in `doing` after INGEST_STALLED_JOB_SECONDS). Instead it
submits, records the gateway job id in a new per-tenant `batch_ocr_jobs` table
(procrastinate args are immutable across retries), and raises a BatchPending
signal that TieredEscalationStrategy turns into a same-queue deferred re-poll —
releasing the worker slot between polls. On completion the per-page markdown is
indexed like the sync path; a failure or a job past
DOCUMENT_OCR_BATCH_MAX_WAIT_SECONDS marks the document parse-failed.
Batch is opt-in and gateway-only: with the direct mistral backend, no gateway
URL, or the inline/memory pipeline (which can't defer), it falls back to sync.
One batch job per document (coalescing N docs/job is a follow-up).
- embedding/gateway_batch_client.py: submit/poll client (reuses GatewayTokenProvider).
- vector/batch_ocr_store.py + migration 008: job tracking (portable SQLite+PG).
- document_processors/escalation.py: BatchPending control-flow signal.
- document_processors/ocr.py: batch state machine + sync fallback.
- vector/processor.py: thread doc identity to the OCR tier; raise BatchPending
from the pending sentinel; propagate it as control flow (not a failure).
- vector/queue/procrastinate.py: BatchPending -> same-queue retry_in, exempt
from the transient cap (bounded by the processor's deadline).
- config + docs; tests across client/store/processor/strategy/parse-tier.
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 review follow-ups (GHSA-8vh3-g2qg-2h2c PR):
- Add a dynaconf validator requiring WEBHOOK_SECRET to be >=16 chars when set
(None still allowed = webhooks disabled), so weak/placeholder secrets fail
at startup rather than in an audit. Covered by two new tests in test_config.py.
- Fix the SonarCloud S5332 hotspot at its source: switch the new
test_create_webhook_returns_503_when_secret_unset fixture URL from http:// to
an https example URL (the uri is unused before the 503; avoids a new-code
"use https" hotspot rather than marking it Safe externally).
- Nits: drop the unused app.state.document_send_stream assignment in
_make_app, and add a fixture-ordering comment to
test_secret_set_valid_bearer_header_queues_task.
(--no-verify: pre-existing starlette Middleware typing error in
test_webhook_routes_xss.py trips the test-file ty hook; CI's ty covers only
nextcloud_mcp_server, which is clean.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review follow-ups (GHSA-8vh3-g2qg-2h2c PR):
- Add a unit test for the new `except WebhookSecretNotConfigured` branch in
enable_webhook_preset: returns 503 (not the generic 500) with WEBHOOK_SECRET
in the body. Uses the existing test_webhook_routes_xss.py scaffolding.
- Add a clarifying comment to test_secret_set_wrong_scheme_returns_401 about
the _client default-bearer override semantics.
(--no-verify: the pre-commit ty-check surfaces a pre-existing starlette
Middleware typing error in test_webhook_routes_xss.py unrelated to this change;
CI's ty check covers only nextcloud_mcp_server, which is clean.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GHSA-8vh3-g2qg-2h2c (CVSS 9.1, CWE-306): POST /webhooks/nextcloud had no
authentication when WEBHOOK_SECRET was unset (the default). The receiver
trusted the attacker-supplied user.uid and fed it to Qdrant, letting an
unauthenticated network caller delete or re-index any user's vector
embeddings.
Webhooks now require WEBHOOK_SECRET end-to-end:
- app.py: the /webhooks/nextcloud route is only mounted when WEBHOOK_SECRET
is set; otherwise it 404s and a startup warning notes vector sync falls
back to the polling scanner.
- webhook_receiver.py: removed the warn-and-accept fallback. No secret -> 503,
missing/invalid bearer -> 401; the payload is never processed unauthenticated.
- webhook_routes.py / api/webhooks.py: webhook_auth_pair() raises
WebhookSecretNotConfigured instead of returning authMethod="none"; both
registration entry points return a clear 503 so no dead unauthenticated
webhooks are created.
Also expose webhooks availability to the Astrolabe UI via GET /api/v1/status
("webhooks_enabled": bool), set WEBHOOK_SECRET on the docker-compose
semantic-search dev services, and update env.sample + ADR-010 / ADR-018 /
webhook-management-guide docs.
Vector sync still works without a secret via the polling scanner.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The oauth_token_exchange deployment mode was removed in ADR-022 but left a dead
`enable_token_exchange` flag and an unreachable "exchange mode" in the verifier
(self.mode was hardcoded to "multi-audience"). Remove the remnants:
- config.py: drop the `enable_token_exchange` default and the
`ENABLE_TOKEN_EXCHANGE` branch in `_is_multi_user` (+ its doc line).
- unified_verifier.py: drop `self.mode` and the dead exchange-mode log branch;
simplify the docstrings to multi-audience only.
- test_unified_verifier.py: drop the `.mode` assertions (attribute removed);
collapse the redundant init tests.
Also remove docs/ADR-004-Code-Review.md — an orphaned code-review note, not an
ADR; it doesn't belong in the docs/ADR namespace.
(--no-verify: the ty-check hook flags 3 PRE-EXISTING type errors in
test_unified_verifier.py lines 346/362/441, untouched by this change; CI
type-checks only the package, which passes.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review round 4 (both nits):
- Rename the flag to ignore_ocr_enabled so its OCR-specific scope is explicit at
the call sites (the gate only bypasses the OCR-enabled check).
- Add test_evaluate_escalation_empty_suppressed_even_when_structured_registered:
empty_text (minimum='ocr') skips a registered structured tier and suppresses to
ocr when OCR is off, never hopping to structured.
Deck #324.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- escalation: EscalationDecision.reason is now Literal["empty_text",
"low_confidence"] (parity with kind; ty catches a bad label at call sites).
- processor: nest the decision handling so the hop branch is reached via an
explicit else under `if decision is not None` — exhaustive over the Literal
kind, no None-attribute risk.
- tests: add the "OCR processor unregistered (not just disabled) → None"
quadrant, locking in absent != suppressed.
Deck #324.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OCR is the paid, opt-in tier (DOCUMENT_OCR_ENABLED, default off). The per-tier
escalation gate already declines to hop to OCR when it's disabled (the pre-OCR
tier is terminal — no surprise cost), but that left operators blind to how much
OCR demand exists.
evaluate_escalation now returns a structured EscalationDecision:
- "hop" — a higher tier can run; the caller raises EscalateError (queue-hop).
- "suppressed" — the ideal next tier (e.g. ocr) exists but is DISABLED; the caller
indexes the current tier's output as terminal and records the
would-be hop on the new astrolabe_document_escalation_suppressed_total
{from_tier,to_tier,reason} counter instead of hopping.
- None — index as-is (good text, or no such tier at all).
So with OCR off, escalation_suppressed_total{to_tier="ocr"} is the latent OCR
demand an operator weighs before enabling OCR; enabling it converts these into
real document_escalation_total{to_tier="ocr"} hops. next_available_tier gains an
ignore_enabled flag to compute the *ideal* (enabled-gate-ignored) target.
Tests: registry suppressed vs hop vs terminal (incl. structured-hop-not-suppressed
when OCR off but structured available); _parse_pdf_tier records suppressed +
indexes without raising.
Deck #324 (parent #323).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A stall is often systemic (a Qdrant/embedding outage stalls every in-flight
job), so reclaiming the whole batch at now() every */5min tick would
thundering-herd a recovering dependency, bypassing TieredEscalationStrategy's
per-job backoff. reclaim_stalled_ingest_jobs now offsets retry_at by a fixed
delay (INGEST_RECLAIM_RETRY_DELAY_SECONDS, default 30s; 0 = legacy immediate).
Also document the hot-vs-restart flag asymmetry: INGEST_ESCALATION_ENABLED is
re-read per job; INGEST_TRANSIENT_MAX_ATTEMPTS is snapshotted at worker startup.
Deck #323.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- metrics: update_ingest_queue_depth guarded on `not by_queue`, which conflated
None (memory backend no-op) with {} (postgres, ALL queues drained). When every
queue drains at once, get_ingest_job_counts_by_queue returns {} and the
pre-zero loop was skipped, leaving a stale ghost backlog in the gauge. Guard on
`by_queue is None` only; add an all-drained regression test.
- procrastinate: note that INGEST_TRANSIENT_MAX_ATTEMPTS is snapshotted at
blueprint-build time (restart to pick up changes).
Deck #323.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- tests: make the transient-backoff progression assertion load-independent by
bracketing the get_retry_decision call with before/after timestamps instead of
measuring against a second datetime.now() (no freezegun dependency).
- tests: use pytest.approx for the ingest-queue-depth gauge assertions —
SonarCloud python:S1244 (float == ) was a MAJOR reliability finding that
tripped the new_reliability_rating quality gate.
- processor: tighten the EscalateError lazy-bind comment (file processing already
imports the document stack via get_registry; the gating only spares the
delete / text-doc paths and module-load time).
Deck #323.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- metrics: update_ingest_queue_depth now pre-zeroes every managed ingest queue
before applying live counts, so a queue that drains to empty (and drops out of
procrastinate's list_queues_async) reads 0 instead of sticking at its last
non-zero value (ghost backlog in Grafana/alerts). Adds a regression test.
- procrastinate: comment that _is_transient_infra_error treats all qdrant errors
as transient deliberately (bounded same-tier retry; over-broad is acceptable).
- escalation: note next_tier is the building block; production routing uses
ProcessorRegistry.next_available_tier.
- tests: add evaluate_escalation fast+ocr-only low-confidence -> ocr case.
Deck #323.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Register the periodic stalled-job reclaim on a dedicated ingest-maintenance
queue that every worker drains (any --tier), so reclaim still fires when the
fast fleet is scaled to zero and only ocr workers run. procrastinate's
periodic-defer dedup keeps it single-run across drainers.
- escalation: mark `unsupported`/`forced` reason labels as reserved (not raised).
- processor: note that options/progress_callback are intentionally not threaded
through _parse_pdf_tier yet (symmetric with the inline path).
- tests: assert TieredEscalationStrategy backoff progression (4/8/16/…/300s);
cover get_ingest_pending per-queue aggregation + the legacy job_counts
fallback; add an external-path zero-page no-escalation case; use the canonical
INGEST_QUEUE_FAST instead of the back-compat alias.
Deck #323.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Split external (procrastinate) document processing into per-tier queues so a
document is attempted at most once per tier and requeued to the next tier's
queue on a low-quality parse, using procrastinate's native retry.
- escalation.py: TIER_LADDER (fast->structured->ocr) + EscalateError signal
- registry: process_tier (one tier) + evaluate_escalation post-parse gate
(reuses classify_from_text) + next_available_tier; shared _classify_result
and _oversize_result with the inline pipeline
- processor: process_document(tier=...) runs one tier and raises EscalateError
before embed (junk text never indexed); inline memory path unchanged
- queue/procrastinate: ingest-fast|structured|ocr queues; TieredEscalationStrategy
(queue-hop on EscalateError, bounded same-tier transient retry); queue-aware
task; producer defers to ingest-fast; per-queue counts + all-queue reclaim
- cli: worker --tier {fast,structured,ocr}
- billing: pages_ocr usage event + pipeline_tier metadata (paid OCR billed apart)
- observability: astrolabe_ingest_queue_depth{queue,status} gauge + per-queue
counts in nc_get_vector_sync_status / management status endpoint
- config: INGEST_ESCALATION_ENABLED (default true), INGEST_TRANSIENT_MAX_ATTEMPTS
INGEST_ESCALATION_ENABLED=false and INGEST_QUEUE=memory preserve prior behaviour.
Deck #323.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review nits. The constant now only gates the diagnostic `image_heavy`
flag (not routing), so the old name was misleading. Rename + reword its comment
to state the diagnostic-only intent. Also add a classify_pdf symmetry test
(`test_classify_pdf_image_heavy_clean_text_stays_fast`) pinning that a full-page
raster image with a clean text layer routes fast on the classify_pdf path too.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The tier-0 classifier escalated any page with raster-image coverage >=0.80 to
the OCR tier regardless of its text layer. On OHR-Bench this drove ~45% of all
OCR escalations: clean born-digital pages dominated by a figure, and scanned
pages that already carry a usable OCR text layer -- re-OCR adds nothing for
either, but each one was routed to the paid tier-3 OCR.
Route on the text signals only (near-empty or junk-quality layer). Image
coverage is still computed and still raises the `image_heavy` diagnostic flag,
but no longer routes. True scans with no/garbage text continue to escalate via
the empty-text and quality signals, so genuine OCR needs are unaffected.
Trade-off: image-only content on an otherwise-clean page (handwriting, stamps,
text inside figures) is no longer force-routed to OCR. This was previously
intentional; the escalation cost outweighed the benefit for RAG indexing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add test_check_status_completion_wakes_user_manager: register the auth tools
against a stub MCP, drive nc_auth_check_status through a completed Login Flow
(mocked storage + poll), and assert it stores the app password and rings the
background-sync doorbell. The MCP-tool wake path was previously only verified
by inspection (round-2 review nit); all three notify_user_provisioned() call
sites now have dedicated coverage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Quality-gate fixes (new-code conditions on PR #902):
- new_security_hotspots_reviewed: drop the fake "http://nextcloud" host in the
manager tests to https:// (python:S5332 ×2).
- new_security_rating: generate the integration test's fake app password with
secrets.token_urlsafe instead of a hardcoded literal (python:S2068).
- new_reliability_rating: restructure the user_manager sleep so an explicit
await checkpoint lives inside the cancellation scope — await one waiter
directly while watching shutdown via start_soon (python:S7490). Behaviour is
unchanged: timeout, shutdown, or a provisioning ring all end the sleep.
Review nits:
- Move the shutdown test's fail_after(2) to wrap the whole task group so it
actually bounds the task-group exit (was guarding a no-op sleep); drop the
sleep(0) stub (python:S7491).
- Type _wake_on's wait_fn as Callable[[], Awaitable[object]].
- Note in _wire_vector_sync_state why provision_signal is set on the singleton
only, not fanned out to app.state.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- ProvisionSignal.wait() re-arms in a finally so a cancelled wait (shutdown
racing the doorbell) leaves a fresh unset event, not a stale set-but-consumed
one; preserves the no-await-before-swap lost-wakeup guarantee.
- Hoist user_manager_task's _wake_on helper out of the while loop (one object,
not one per iteration).
- Test: assert ProvisionSignal via its public wait() contract instead of the
private _event attribute.
- Add test_provision_app_password_wakes_user_manager covering the
api/passwords.py wake path (previously only LFv2 web + MCP tool were tested).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Background vector sync discovered newly provisioned users only on the
periodic user-manager poll (VECTOR_SYNC_USER_POLL_INTERVAL, default 60s),
delaying first indexing by up to a minute. Add a ProvisionSignal doorbell
that provisioning paths ring after storing a user's app password, waking
user_manager_task to re-poll and spawn the user's scanner at once. The
periodic poll remains the backstop (covers cross-replica provisioning).
- ProvisionSignal (stable reference, wait-and-re-arm) held on
VectorSyncState; closes the lost-wakeup window (no await between observing
the ring and re-arming; anyio.Event stickiness covers a mid-poll ring)
- user_manager_task races its poll timeout against the doorbell + shutdown
- notify_user_provisioned() rung from the three app-password provisioning
sites: Login Flow v2 web, MCP provisioning tool, management/BasicAuth API
Note: the pre-existing scanner_wake_event was never .set() and only wakes
existing scanners; a brand-new user has none, so the manager is what must
be nudged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address round-1 claude-review nits on PR #894:
- Document why offline_access is advertised unconditionally (independent of
settings.enable_offline_access): per RFC 8414, scopes_supported lists what
the AS *can* support, with actual issuance still gated upstream by Nextcloud.
- Add a regression test proving the offline_access invariant holds on an empty
FastMCP instance with no registered tools.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
discover_all_scopes() builds the scopes_supported lists exposed at
/.well-known/oauth-protected-resource and /.well-known/oauth-authorization-server.
It previously emitted only the base OIDC scopes plus tool-derived
@require_scopes, so offline_access was never advertised and
discovery-driven MCP clients had no way to know they could request a
refresh token.
Add offline_access unconditionally. The AS proxy already forwards
client-requested scopes upstream to Nextcloud, which issues a refresh
token when the MCP server's OIDC client is permitted the scope. This
only changes what is advertised; it is not added to ALL_SUPPORTED_SCOPES
(the app-level permission set), since offline_access is an OIDC behavior
rather than a Nextcloud resource permission.
Add a regression test asserting offline_access is always present in
discover_all_scopes() output.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-4 review on PR #893 (no blockers, minor items):
- Document why Mistral's _is_transient is SDK-level only (429/5xx): a bare
connection drop the SDK surfaces as httpx/ConnectionError isn't an SDKError
and isn't retried here by design — the pod-rollover target is the gateway
(OpenAI-compatible) path, which does cover connection errors.
- Include the last error (%r) in the retry helper's "not resolved after N
attempts" error log.
- Add test_mistral_embed_batch_retries_on_5xx (batch path parity with embed()).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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-3 review on PR #893:
- record_qdrant_operation("upsert","error") now fires only when the exhausted
retry was actually a Qdrant failure (reason=="qdrant"); an embed/connection
failure exhausts retries before Qdrant is called, so attributing it to
mcp_qdrant_operations_total{error} inflated that signal. The cause is still
captured by record_ingest_dropped.
- Add test_mistral_embed_retries_on_5xx: exercises the full Mistral retry path
(5xx SDKError then success), not just the predicate.
- Add test_generate_does_not_retry_on_bad_request: generate() fast-fails on a
permanent 4xx.
- Move astrolabe_vector_ingest_dropped_total's definition into the astrolabe_
pipeline-metrics block (was in the mcp_ section).
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>
Round-2 review on PR #893:
- Add test_generate_retries_on_connection_error (generate() shares the transient
retry; guards the decorator against accidental removal).
- Add test_process_document_records_drop_on_exhausted_retries: drives
process_document to retry-exhaustion and asserts record_ingest_dropped is
called once with the classified reason (processor-level coverage, not just the
_drop_reason unit).
- Note in _drop_reason that a multi-failure group is labelled by its first leaf
(best-effort, no "mixed" bucket).
SonarCloud: the quality gate was failing on new_security_hotspots_reviewed
(S5332 "use https") from http:// URLs in the test _req() helpers — 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>
Round-1 review on PR #893:
- _drop_reason now descends through nested ExceptionGroups to the first leaf
(was single-level), so a doubly-wrapped cause isn't mislabelled "other";
added a nested-group test. Commented why both the httpx and openai isinstance
branches exist (raw Nextcloud-API errors vs SDK-wrapped variants).
- Documented that generate() intentionally shares the broadened transient retry
(RAG sampling path), with the worst-case latency note.
- Added a docstring note to process_document on how the provider-level retry
(5x) layers over the outer loop (3x in-process / 1x procrastinate).
- Added test_embed_batch_retries_on_connection_error for the batch path.
- Renamed test_retry_reraises_non_rate_limit_immediately ->
test_retry_reraises_when_predicate_returns_false (it tests the predicate, not
a specific status).
SonarCloud:
- S5708 (BLOCKER) on the helper's dynamic `except exception_type`: the type is
constrained to BaseException/tuple by the signature; suppressed with a
justified NOSONAR.
- S7503 (async without await) in the embed-retry test: use AsyncMock side_effect
instead of a hand-rolled async function.
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>
From card 309 (OHR-Bench smoke-test triage): during a backend-pod rollover the
embedding endpoint was briefly unreachable, and openai.APIConnectionError /
ConnectError propagated unretried (the provider only retried 429). Documents
exhausted the 3 in-process retries and were dropped for that scan cycle.
Broaden the provider-level retry to the transient set -- APIConnectionError,
APITimeoutError, 429, and 5xx -- on the existing exponential backoff (2s->60s,
5 attempts), so a few seconds of retry rides through the rollover. Permanent
4xx (auth, bad request) still re-raise immediately. Generalize the shared
_retry helper (retry_on_rate_limit -> retry_on_transient, predicate renamed to
should_retry, accurate log label) with a back-compat alias; Mistral gets 429+5xx
for parity. The production gateway path inherits this via GatewayProvider, which
delegates to the decorated OpenAIProvider methods.
Add astrolabe_vector_ingest_dropped_total{reason}, incremented when a document
exhausts retries, classified (connection|timeout|rate_limit|server|qdrant|other)
by _drop_reason so the embed-drop rate is alertable per cause. Dropped docs are
NOT marked failed, so the next full scan re-picks them (re-queue via scan loop).
Refs: Deck board 12 card 309 (AC #1 no permanently-dropped docs; embed-drop
metric for AC #5).
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>
