Address claude-review round 2 on #922:
- Legacy `ingest-ocr` tier resolution (important 1): document why it deliberately
resolves to `fast` rather than mapping to `ocr-upstream` — stranded pre-split
jobs re-extract empty and re-escalate via the ladder to the cheap
`ocr-incluster` rung, keeping them OFF the paid upstream rung. Added a
tier_for_queue(LEGACY_INGEST_QUEUE_OCR) == "fast" assertion.
- Double get_settings() in `_get_batch_client` (important 2): bind once to a local.
- Stale docstrings (important 3): OcrProcessor (serves both rungs now),
_tier_available (both OCR rungs gated), evaluate_escalation (targets
ocr-incluster, falls through to ocr-upstream).
- Misconfigured model_setting (nit 5): OcrProcessor.__init__ raises ValueError on
an unknown settings attr (fail-fast at startup vs AttributeError mid-OCR); also
removes the dynamic-getattr static-analysis smell SonarCloud flagged.
- Redundant guard (nit 4): kept `and ocr_tier is not None` — it's required for ty
to narrow ocr_tier to str for record_document_escalation; added a comment.
- Test gap (nit 6): added a test pinning the CURRENT incluster-failure ->
tier-1 fallback (does NOT cascade to upstream) so the future 503-escalation
change is an explicit diff.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review on #922:
Blocking — test coverage for the new tier2 rung:
- test_registry_tiering.py: inline empty-text routes to ocr-incluster before
ocr-upstream; only-incluster-enabled routes to incluster; disabled-incluster
skips to upstream; evaluate_escalation empty_text hops to ocr-incluster (and
falls through to upstream when incluster off); next_available_tier walks the
full fast→structured→ocr-incluster→ocr-upstream ladder + ignore_ocr_enabled
ideal-target.
- test_escalation_signature.py: enabling document_ocr_incluster_enabled changes
the dead-letter signature (independent of the upstream rung).
- test_tiered_escalation_strategy.py: structured→ocr-incluster hops to
INGEST_QUEUE_OCR_INCLUSTER; tier_for_queue covers the in-cluster queue.
Important — real fixes:
- registry.py: run scan detection (image_coverage_per_page) when EITHER OCR rung
is enabled, not just the upstream one — a tenant with only in-cluster OCR on
was missing image-coverage scan signals.
- ocr.py: the gateway_only (in-cluster) processor never enters batch mode — the
GPU is synchronous/low-latency; batch OCR is the upstream Mistral async path.
_get_batch_client short-circuits to None. Covered by a new test.
Nit:
- cli.py: worker --tier help lists ocr-incluster/ocr-upstream as separate fleets.
Left as-is: the lazy anyio.Lock init in OcrProcessor — instances ARE created at
module import (document_processors/__init__.py), so deferring lock creation off
import time is still required; moving it into __init__ would reintroduce the
import-time-primitive issue the comment guards against.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Insert a configurable in-cluster OCR rung into the escalation ladder (Deck #353):
a tier2-eligible doc is OCR'd on the on-demand burst GPU before falling through to
paid upstream OCR. The in-cluster backend is reached ONLY via the embedding gateway
(model prefix routes to the GPU over the tailnet) and is a config value (default
surya/surya-ocr-2, swappable to e.g. lightonocr) — never hard-coded.
Ladder: fast -> structured -> ocr-incluster -> ocr-upstream
(queues ingest-ocr-incluster / ingest-ocr-upstream).
- escalation.py: 4-tier ladder; in-cluster flag folded into the dead-letter signature.
- ocr.py: OcrProcessor(name, tier, model_setting, gateway_only); build_ocr_backend(
..., model=, gateway_only=) — gateway_only forces the gateway backend (never the
direct Mistral fallback), disabling the tier with a warning if no gateway URL.
- registry.py: per-rung enable map; scanned docs target minimum="ocr-incluster";
inline path runs the cheapest available OCR rung.
- procrastinate.py: two OCR queues; legacy ingest-ocr kept as a drain target.
- config.py: DOCUMENT_OCR_INCLUSTER_ENABLED (off) + DOCUMENT_OCR_INCLUSTER_MODEL.
- __init__.py: register the two OCR instances; vector/processor.py: pages_ocr
metered for the upstream (paid) rung only; cli.py: new --tier choices + legacy drain.
- metrics.py: zero the legacy ingest-ocr queue gauge during rollout.
- tests: migrated to the split ladder + new tests (gateway-only forcing, per-tier
model incl. lightonocr override, no-hard-coded-surya guard). 1792 pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 10 (Option B): pin the empty-scope contract for
userinfo-validated tokens in test_mgmt_opaque_userinfo_fallback_accepted_despite_allowlist,
so a future @require_scopes on a management endpoint that would silently reject
cross-client callers is caught by a test rather than only the docstring.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 8 on #919:
- Security-model docstring: note that opaque cross-client tokens authenticate
via the userinfo liveness check (not JWKS/expiry) and bypass the client
allowlist, with per-user authz as the gate.
- Remove the redundant `if not payload: return None` after the JWT/opaque
branches (both already return None on failure) — replace with a comment.
- Add test_mcp_path_does_not_use_userinfo_for_opaque_token to pin that the
userinfo fallback is management-path-only (MCP path still 401s).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 7 nits on #919:
- Add test_validate_via_userinfo_rejects_non_http_scheme — a non-http(s)
userinfo_uri is refused before any request (covers the SSRF scheme guard).
- Docstring caution on _validate_via_userinfo: userinfo-validated tokens carry
empty scopes, so management endpoints must not gate on scopes for this path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 6 (LGTM) nits on #919:
- test_userinfo_token_cached_with_short_ttl and
test_userinfo_token_with_exp_uses_real_expiry call only the sync
_create_access_token_with_cache_key — declare them as plain def (no await).
- Comment the userinfo_uri guard in _validate_via_userinfo as defensive /
direct-call support (the management caller already gates on userinfo_uri).
Left as-is: the hasattr(settings, "userinfo_uri") guard — kept to mirror the
adjacent introspection_uri block (consistency requested in round 2). The
_verify_mcp_audience metric-when-unconfigured note is a pre-existing, out-of-
scope item for a follow-up.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 5 on #919:
- The "userinfo has no exp; caching for Ns only" log fired on every fresh
userinfo validation (userinfo never returns exp) — downgrade WARNING → DEBUG;
the bounded-staleness window is already documented on _validate_via_userinfo.
- Add test_introspection_timeout_falls_through_to_userinfo: drives a real
introspection timeout (httpx.TimeoutException on the POST, caught inside
_introspect_token → None) through to a successful userinfo validation,
pinning the documented error fall-through end to end.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 4 on #919:
- Functional concern: document that _introspect_token returns None for both an
active=false response (the cross-client case we must handle) AND a network
error, so both fall through to userinfo. This is safe — userinfo is itself an
authoritative live check, so a flapping introspection endpoint can't cause an
invalid token to be accepted.
- Observability nit: only record a ("userinfo", ...) metric when userinfo was
actually attempted (userinfo_uri configured); a no-validators-configured
opaque token now returns None without a misleading userinfo-failure metric.
Added test_opaque_rejected_when_no_validators_configured.
- Added a comment on the post-validation cache re-read explaining why the entry
is always present (write-then-read with no await between).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 3 on #919:
- Log spam: the userinfo allowlist-relaxation notice fired at WARNING on every
request (incl. cache hits — frequent Astrolabe polling). Warn once on fresh
validation; cache-hit re-validations now log at DEBUG.
- Test: add coverage for a userinfo response that DOES carry `exp` — the real
token expiry must win over the short userinfo TTL.
Not changed:
- USERINFO_URI auto-discovery: already auto-populated from the OIDC discovery
document in app.py (settings.userinfo_uri = discovery["userinfo_endpoint"],
mirroring jwks_uri/introspection_uri), so OIDC_DISCOVERY_URL deployments need
no extra env var. The reviewer's note only inspected config.py.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 2 on #919:
- Anti-forgery: the `_auth_via_userinfo` allowlist-bypass flag is now sourced
ONLY from an explicit in-process `via_userinfo` argument (derived from how
the token was validated), never from the IdP payload. The payload claim is
stripped from the cached entry, so a malicious introspection/userinfo
response can't forge the bypass. Added a regression test.
- SSRF (CWE-918): guard the userinfo_uri scheme (http/https) before the request
— documents the trusted-source assumption and fails fast on misconfig.
- Introspection-unconfigured: only attempt introspection (and record its
metric) when an introspection endpoint is configured; otherwise go straight
to userinfo. Avoids mislabelled introspect-invalid metrics. Added a test.
- Tests: cache-hit test now seeds via a real first call (behavior, not cache
internals) and asserts the network is probed once; short-TTL test uses the
explicit via_userinfo arg; moved hashlib usage out.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses round-2 review on PR #920:
- Only dead-letter a terminal failure when the file has an etag to
content-address the marker; without one, fall back to the legacy per-user
placeholder mark (an etagless marker is unmatchable). + test.
- _dead_letter_filter now also matches is_placeholder=True (redundant with
dead_letter=True but lets Qdrant use the is_placeholder payload index).
- TODO(deck-349) documenting the dead-lettered-then-deleted orphan-marker leak
(out of scope; needs a marker sweep or TTL field) per reviewer.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses round-1 review on PR #920:
- Delete path now clears the file's dead-letter marker after
release_document_for_user (whose principal-based filter misses the
user-agnostic, principal-less marker), preventing orphan-marker accumulation
for dead-lettered-then-deleted files.
- Oversize PDFs (rejected by the pre-parse size guard, no pipeline_tier stamped)
are now treated as terminal regardless of failing_tier -- no tier can parse an
oversize file -- so they dead-letter instead of falling to the legacy per-user
mark on the inline path.
- Gate the success-path clear on a non-empty etag (an etag-less file can never
have a marker, mirroring is_dead_lettered's early return).
- dead_letter.py: payload typed dict[str, Any] (CLAUDE.md).
Tests: oversize-terminal dead-letter and delete-path marker clear.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A pathological PDF (a 206-page ChronoScan scan with ~3400 JBIG2/JPX images)
jammed a tenant's structured ingest worker in an infinite reprocess loop,
re-burning a 120s pymupdf4llm parse (and occasionally OOM-racing the 2Gi pod)
every few minutes.
Root cause: the per-user placeholder "failed" mark could not stop the loop. The
placeholder point ID is user-agnostic (uuid5("file:<doc_id>:placeholder")) but
the scanner's freshness gate, query, and status update all filter by user_id.
For a file visible to several users the single shared placeholder's user_id is
overwritten by whoever scanned last, so every other user's scan sees "no record"
and re-queues -- an N-user ping-pong that never honours the failed status.
Fix: when a parse fails terminally (no higher escalation tier available, e.g.
structured with OCR off) record a durable, content-addressed, user-agnostic
dead-letter marker (mirrors vector/sharing_state.py). The scanner consults it
tenant-wide for every user and skips re-queuing until the content (etag) OR the
escalation-tier set (tiers_sig -- e.g. OCR enabled) changes, so the document is
attempted once per content-version instead of forever.
- new vector/dead_letter.py: mark/is/clear, content-addressed marker carrying
is_placeholder=True (inherits search exclusion) + dead_letter=True
- escalation.escalation_tiers_signature(settings): retry-on-tier-change key
- processor: dead-letter terminal failures, clear on successful (re-)index
- scanner: user-agnostic is_dead_lettered skip beside claim_existing_index
- placeholder: exempt dead_letter markers from the orphan sweep (durability)
- metrics: astrolabe_document_dead_lettered_total{reason}
Deck #349.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 1 on #919:
- Security: userinfo responses carry no `exp`, so userinfo-validated opaque
tokens were cached for the 1h default TTL — a revoked/expired token could be
honored for up to an hour. Cache them for `userinfo_cache_ttl` (5 min)
instead, and document the bounded-staleness window in the docstring.
- Metrics: when introspection AND userinfo both fail, record
("introspect","invalid") + ("userinfo","invalid") separately and set
validation_method="userinfo" before the userinfo call so a userinfo
exception caught by the outer handler is attributed correctly.
- Style: use the hasattr(...) + truthy pattern for userinfo_uri, matching the
introspection block above it.
- Tests: cache-hit allowlist bypass for via-userinfo tokens; short-TTL
assertion; userinfo timeout / connect-error / malformed-JSON fail-closed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The management API (used by the Astrolabe PHP app for /api/v1/apps and
/api/v1/webhooks) only accepted JWT access tokens. Opaque tokens were
sent to Nextcloud's oidc introspection endpoint, which returns
`active: false` for tokens minted for a *different* OIDC client (e.g.
Astrolabe) even when they are live — so every call 401'd. This surfaced
on the nx101294 tenant: webhook setup failed and the webhook-preset UI
(including the Files preset) showed empty, because getWebhookPresets
errors out before its `files`-always-available filter runs.
Add a userinfo-endpoint fallback in UnifiedTokenVerifier: when
introspection reports an opaque token inactive, validate it against the
discovered userinfo_endpoint (a 200 with a `sub` proves a live bearer
regardless of issuing client). userinfo returns no client_id/scope, so
such tokens are stamped `_auth_via_userinfo` and the ALLOWED_MGMT_CLIENT
allowlist is relaxed for that path only — authorization is still
enforced per-user (token sub == requested resource owner) by every
management endpoint. JWT and introspection paths are unchanged and still
enforce the allowlist.
Also bumps the astrolabe submodule to 0.29.0 (the deployed version that
exhibits the issue).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The client registry derived display names from a hardcoded map of
"well-known" MCP clients (claude-desktop, claude-ai, continue-dev,
zed-editor, vscode-mcp). This baked a recognized-client list into the
server even though admission already requires explicit opt-in via
ALLOWED_MCP_CLIENTS (fail-closed when unset).
Mirror the management-API surface (ALLOWED_MGMT_CLIENT), which has no
built-in client list: remove the map and derive the display name
generically from the client_id. Default remains none; clients must be
added explicitly and DCR stays off unless ENABLE_DCR=true.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address round-4 review on PR #914:
- glyph_corruption_ratio <= 0 now disables the signal (previously `control_ratio
> 0` fired on any single C0 control byte), matching the "0 disables" convention
used elsewhere (document_max_pdf_size_mb) and the config comment. Add a
zero-disables test.
- Correct the document_escalation_suppressed_total comment: corrupt_glyphs CAN
appear there in the narrow case where structured is unregistered and OCR is
registered-but-disabled (evaluate_escalation follows minimum="structured" past
the missing rung to a gated-off OCR). Add a test for that suppressed decision.
- Add a test for the double-corruption edge: a structured re-extract that is also
glyph-corrupt escalates structured->ocr with reason corrupt_glyphs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address round-3 review on PR #914:
- When a glyph-corrupt doc's structured rung is NOT registered, the inline path
now falls through to OCR (with reason corrupt_glyphs), mirroring the external
next_available_tier instead of silently keeping the fast result. A structured
parse FAILURE remains terminal (tracked via structured_failed), matching the
external path which does not escalate a failure. Added a debug log for the
unregistered case and "(OCR not attempted)" to the failure warning.
- Tests: inline + external glyph-corrupt fallthrough to OCR when structured is
unregistered; glyph-corrupt + junk-quality both-flags precedence (structured
wins over the bad_text_layer/ocr route).
- Note the total_chars>0 mutual-exclusion with the scanned branch in
_route_from_signals.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address round-2 review on PR #914:
- Add corrupt_glyphs to the document_classifier_flag_total label comment (it is
a live flag value emitted by record_document_classification).
- Mirror the full_text-vs-sampled control-ratio NOTE into classify_pdf so the
diagnostic path's under-detection trade-off is documented in place.
- Add test_classify_pdf_glyph_corrupt_routes_structured for routing symmetry on
the standalone classify_pdf path.
(SonarCloud quality gate is green — the prior S1244 finding was fixed last round.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address round-1 review on PR #914:
- Attribute the OCR hop in a fast->structured->ocr inline cascade to
from_tier="structured" (not a second "fast" escalation), so
astrolabe_document_escalation_total per-tier counts stay accurate.
- Add test_inline_fast_structured_ocr_cascade pinning that two-hop path and the
metric attribution.
- Note in classify_from_text that its doc-level control ratio is over full_text
(all pages), not the sampled subset classify_pdf uses.
- Clarify that corrupt_glyphs never lands in the suppressed-escalation counter.
- Dedupe the glyph-corrupt test string into tests/fixtures/glyph_corruption.py.
- Use pytest.approx for the control-char-ratio zero checks (SonarCloud S1244).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The fast (pypdfium2) extractor can leak raw glyph codes on subset fonts with a
broken /ToUnicode CMap. The result scores high on the existing text-quality
heuristic -- a uniform glyph/Caesar offset preserves whitespace and token
lengths -- yet is unsearchable. The structured (pymupdf) tier extracts the same
pages correctly.
Add a language-agnostic C0-control-character-ratio signal to the tier-0
classifier that detects this corruption and routes the document to a new
`structured` recommended_tier. Wire the fast->structured hop on the inline path
and generalise it so a low-quality-but-non-empty layer also tries structured
before OCR -- the inline and external ingest modes now follow the full
fast->structured->ocr ladder identically. A scanned / no-text-layer document
(total_chars == 0) still shortcuts straight to OCR, since a text extractor
cannot recover a pure raster.
New per-tenant tunable DOCUMENT_GLYPH_CORRUPTION_RATIO (default 0.02); escalation
metrics gain a `corrupt_glyphs` reason label.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address round-1 review on #913 and the SonarCloud new_reliability_rating gate:
- credential_cleanup_task no longer catches the cancellation exception
(Sonar python:S7497). A task-group cancel must propagate for structured-
concurrency teardown; graceful shutdown still flows through shutdown_event,
so the sleep no longer needs a cancel/break.
- Parametrize the scanner self-heal tests over 401 AND 403 (handled
identically at both call sites) and add a test that a failing periodic
sweep is logged non-fatally and does not crash the task.
- Log the stored-user count before the startup sweep (operability signal),
add a debug line when the credential row was already gone, and document
the at-most-one extra-401 convergence in _remove_stale_credential.
Refs Deck #198.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deleted/disabled Nextcloud users left their app_passwords row in storage,
so user_manager_task re-spawned their scanner every poll interval only to
401 again — an endless re-spawn/auth-failure loop (observed on
tenant-blackbox-demo: ~534 respawns/3h, matching the 60s poll interval).
- Delete the stored app password on a hard 401/403 in user_scanner_task
(both the pre-validation and in-scan-loop paths), breaking the re-spawn
loop at the source so the user-manager stops recreating the scanner.
- Add a periodic credential_cleanup_task backstop (hourly) that sweeps
cleanup_invalid_app_passwords for anything the per-scanner path misses.
- Run the startup cleanup for all deployment modes: drop the stale
`not oauth_enabled` guard so login_flow tenants (the cloud default) are
covered. NOTE: login_flow startup now makes one concurrent OCS
validation call per stored user before readiness.
Refs Deck #198.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 style note: replace `return None` with a comment-only intentionally
empty body for the _state_admin_can_purge stub.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address round-1 review nits:
- document why _ocs_capabilities omits the rest of the OCS envelope (Pact V4
allows extra provider-side keys; pin only astrolabe's own block)
- use the module-level _BROKER_READY in the broker-source guard instead of
re-checking the three env vars inline
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a Pact consumer test for capabilities.allowed_doc_types ->
NextcloudClient.capabilities() -> GET /ocs/v2.php/cloud/capabilities, pinning
the astrolabe.semantic_search.enabled_doc_types block the search/scan/purge
gates read. Covers the two meaningful provider states: some sources approved
(parsed to the allow-set) and every source disabled (empty frozenset, distinct
from the fail-open None). Produces the nextcloud-mcp-server -> astrolabe pact.
On the provider side (astrolabe's consent-purge pact), register the
"an admin can purge indexed documents" provider state and opt the broker source
into pending pacts, so that authenticated contract reports as pending instead of
failing provider verification until the live-stack auth test-hook is stood up
(ADR-029 phase 4). Already-verified interactions (GET /api/v1/status) stay
blocking.
---
_This PR was generated with the help of AI, and reviewed by a Human_
- route test for purge_doc_types raising on total failure -> 500
- route test for doc_types list containing non-strings -> 400
- reword the capabilities move_to_end comment (no-op on new keys; needed only
for the expired-key in-place update)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- vector_sync route: rename the response dict from `body` to `resp` so it no
longer shadows the request `body` (maintenance trap)
- scanner: comment the intentional files-vs-text purge timing asymmetry
- tests: add the all-text-types-disabled backstop case (empty allow-set)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- purge route: include a "failed" key in the 200 body listing requested doc
types that were not purged, so Astrolabe knows consent isn't yet enforced
for them (scanner backstop still catches up)
- tests: add @pytest.mark.unit / module-level pytestmark to the new test
modules so they run under `pytest -m unit`; add a partial-failure route test
- capabilities: comment why the cache is keyed per-user despite a global value
- semantic/scanner: doc/comment clarifications (sorted-order, eviction timing)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- tests: cover the process_document consent gate (drops an admin-disabled
index task with record_ingest_dropped("admin_disabled"); allows approved)
- scanner: _consent_backstop_done is now an insertion-ordered dict and evicts
the oldest entries to half capacity on overflow, so a bound hit re-fires the
backstop for only the oldest markers instead of the whole fleet at once
- semantic: reword the short-circuit log (consent, not installation)
- capabilities: comment why move_to_end is needed after an expired-key update
- test: assert the global purge delete-filter is owner-agnostic (doc_type only);
fix a pre-existing ty error on UnexpectedResponse(headers=None) in the file
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- app.py: register /api/v1/vector-sync/purge only when vector_sync_enabled, so
it returns 404 (not a 500 from get_qdrant_client) when sync is off
- scanner: bound _consent_backstop_done so a long-running multi-tenant process
with user churn can't grow it without limit (clears on overflow)
- purge route: distinct 400 for a missing doc_types key; enforce the admin
check even for an empty no-op request (destructive route)
- tests: missing-key 400, admin-gated empty no-op, non-admin empty 403
The _consent_narrowed_doc_types precondition is enforced by its non-Optional
frozenset[str] signature (ty rejects a None caller). The httpx.BasicAuth
SonarCloud hotspot matches the existing webhook routes (false positive,
credential from the app-password store) — left consistent for UI triage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- scanner: gate the consent backstop with a per-(user,doc_type) one-shot
marker so a standing admin-disable doesn't re-enqueue idempotent deletes
every scan tick; the marker clears when the type is re-enabled. Derive
_TEXT_BACKSTOP_DOC_TYPES from INDEXED_DOC_TYPES so new indexed types are
covered automatically
- semantic: extract _consent_narrowed_doc_types so the search-side narrowing
is unit-testable; add tests for restrict/intersect/disjoint/empty
- purge route: cap doc_types length (abuse guard) -> 400
- tests: one-shot + re-enable backstop, too-many-doc_types 400
Deferred (noted on PR): per-document allowed_doc_types call is cache-hot;
purge "last error wins" — both logged. SonarCloud broad-except hotspots are
deliberate (noqa BLE001), reviewable in the UI.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- purge route: 400 (not 500) on a valid-JSON non-object body
- scanner: backstop-purge admin-disabled note/news_item/deck_card points
(their deletion-tracking lives inside the skipped scan_* fns), mirroring the
files path; gated on a concrete allow-set so fail-open never deletes
- processor: record_ingest_dropped("admin_disabled") so consent-skipped index
tasks are observable/alertable
- app.py: list /api/v1/vector-sync/purge in the endpoints log line
- capabilities: drop empty-string doc types; return frozenset throughout
- purge: document the count-before-delete approximation
- tests: non-object body -> 400, ProvisioningRequiredError -> 428, cache TTL
expiry refetch, and the scanner consent backstop
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Consume the astrolabe.semantic_search capability as the source of truth for
which content sources an admin has approved for semantic search, and enforce
it independently of Astrolabe (this server queries Qdrant directly).
- capabilities.py: cached per-user reader for enabled_doc_types (TTL+LRU,
fail-open so older Astrolabe / transient OCS errors don't break search)
- semantic search: intersect requested doc_types with the allowed set;
restrict to the allowed set when none requested; short-circuit when empty
- scanner: skip disabled sources during discovery (files discovery yields
nothing when disabled, so the existing grace-period reconcile purges them)
- processor: drop near-real-time index tasks for disabled doc_types
(webhook events bypass the scanner gate); deletes always proceed
- vector/purge.py + POST /api/v1/vector-sync/purge: admin-only global
delete-by-doc_type, called by Astrolabe when a source is disabled so
consent is binding on data-at-rest
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the consumer-driven Pact for the gateway's async batch OCR routes, consumed
by GatewayBatchOcrClient. The embedding gateway is a separate provider
(astrolabe-cloud-gateway) from the existing `astrolabe` credentials pact, so it
gets its own fixture + pact file.
Interactions (only the fields the single-document client reads are pinned, so the
contract is robust to the gateway's additive OcrBatchJobOut fields):
- POST /v1/ocr/batch -> 202 { job_id } (namespaced <provider>/<id>)
- GET /v1/ocr/batch/{job_id} -> pending / succeeded (per-page markdown) / failed
The gateway is unauthenticated today, so no bearer is sent (matching the
M2M-optional client). Provider-side: this publishes a pact the gateway's
verification job must now satisfy — it needs provider-state handlers
(pending/succeeded/failed jobs) + a Mistral stub on the astrolabe-cloud-website
side (its verification was a deliberate no-op until a consumer pact existed).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 6 review (PR #910):
- Add the missing propagation test: a transport error (httpx.ConnectError) from
batch submit() propagates out of process() rather than being caught by the sync
OCR try/except or falling back to a sync transcription — guards the intentional
"opted into batch → procrastinate retry, not sync fallback" asymmetry.
- Move the batch-test module imports (BatchPollResult, batch_ocr_store) to the
top of test_ocr_processor.py, dropping the mid-file `# noqa: E402`.
Deferred (reviewer: not actionable for this PR): extracting a lazy-init helper
for the parallel _backend / _batch_client resolution quadruplets.
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 5 review (PR #910), all nits, no blockers:
- _result_from_success: a succeeded job with `pages=[]` (empty list, not just a
missing key) is now a per-document failure ("no pages returned") instead of a
silent 0-chunk success. Test added.
- Comment the deadline-expiry path: the gateway-side job isn't cancelled (no
cancel endpoint at this layer) — it's reaped by the gateway file purge; we just
stop polling it.
- Drop the vestigial status="pending" from the BatchOcrJob test fakes (the column
was removed in round 4; BatchPollResult.status fakes are untouched).
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 4 review (PR #910), no blockers:
- poll(): a 2xx body with no `status` now fails fast (logged) instead of being
treated as perpetually pending until the deadline; defensive page index
(`p.get("index", i)`) so a malformed page degrades rather than KeyError-ing.
- Document on poll() that job_id is namespaced (embeds "/") so the gateway route
must be a path-capture param (GET /v1/ocr/batch/{job_id:path}).
- Drop the vestigial `status` + `updated_at` columns from batch_ocr_jobs: a row
only ever exists while pending (terminal jobs are deleted) and the live status
comes from a fresh poll, so a stored mirror was permanently "pending" /
redundant with submitted_at. Simplifies the migration, store, and dataclass.
- Tests: submit() ValueError on missing job_id; poll() missing-status → failed.
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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>
