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>
Round-8 review: env.sample.oauth-multi-user omitted NEXTCLOUD_PUBLIC_ISSUER_URL,
which configuration.md marks required for login_flow — a user working from the
template alone would hit the "Login URL points to localhost" failure. Add it
(with a troubleshooting pointer) and give it + NEXTCLOUD_MCP_SERVER_URL a
"REQUIRED: PUBLIC URLs" section header for consistency with the rest of the file.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-7 nit: align the TOKEN_ENCRYPTION_KEY placeholder in login-flow-v2.md
(`<fernet-key>` / `<your-fernet-key>`) with env.sample.oauth-multi-user's
`<your-encryption-key>` so copy-pasters don't see a mismatch.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-6 review: in env.sample.oauth-multi-user, TOKEN_ENCRYPTION_KEY and
TOKEN_STORAGE_DB sat under "OPTIONAL: SEMANTIC SEARCH", but they're required for
any login_flow deployment (per-user app passwords must be persisted). Move them
into a dedicated "REQUIRED: APP-PASSWORD STORAGE" block with a pointer to
docs/login-flow-v2.md#setup so a login_flow-without-semantic-search user copying
the template doesn't miss them.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- settings.toml.example: drop the stale `enable_token_exchange = false`
deprecated-alias line (the key was removed from config.py _DEFAULTS).
- configuration-migration-v2.md: add a Quick Reference row + note that
`ENABLE_TOKEN_EXCHANGE` was removed and is now ignored; use
`MCP_DEPLOYMENT_MODE=login_flow` instead.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-4 review: ADR-005 (Status: Implemented) still described the token-exchange
mode (Option 2 / ENABLE_TOKEN_EXCHANGE) as an active option. Add a note to the
Implementation Note section clarifying it was removed in the ADR-022/023
consolidation and only multi-audience mode ships — consistent with the ADR-004
deprecation in this PR.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- login-flow-v2.md: add commented-out NEXTCLOUD_OIDC_CLIENT_ID/_SECRET (with a
"production: register a static client" note) to the Docker Compose excerpt so
copy-pasters of the rendered snippet don't fall into the #907 DCR-expiry trap.
- ADR-004: rename "## Implementation Status" -> "## Historical Implementation
Notes" and add a banner clarifying the steps were never completed and the
ENABLE_TOKEN_EXCHANGE symbols no longer exist (the design was superseded).
- env.sample.oauth-multi-user: angle-bracket the TOKEN_ENCRYPTION_KEY
placeholder for consistency with the OIDC client placeholders.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 review follow-ups in unified_verifier.py:
- module-level docstring still described "two compliant OAuth modes" incl.
token exchange — rewritten to multi-audience only.
- removed the stale "# Both modes do the same validation" inline comment in
verify_token().
(--no-verify: same pre-existing ty errors in test_unified_verifier.py as prior
commits; CI type-checks only the package, which passes.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review follow-ups:
- Troubleshooting "Access forbidden": note that existing users must re-authorize
once after switching to a static client (stored sessions were issued to the
now-deleted DCR client).
- Default IdP setup: explain that the `/mcp` resource identifier works because
`_has_mcp_audience` accepts both the bare server URL and the `/mcp` form.
- env.sample.oauth-multi-user: use angle-bracket placeholders
(`<your-client-id>`) to match the template convention and fail loudly if
copied verbatim.
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>
The OAuth token-exchange deployment mode was removed (ADR-022) and has no
implementation — only a vestigial `enable_token_exchange` flag remains. Its
documentation still presented it as a usable mode, which misleads self-hosters.
The only supported deployment modes are single_user_basic, multi_user_basic,
and login_flow.
Token-exchange removals (how-to/config for a removed mode):
- delete docs/CRITICAL-TOKEN-EXCHANGE-PATTERN.md
- delete docs/oauth-architecture-comparison.md (orphaned; labelled the removed
pass-through mode as "current implementation")
- env.sample: drop the "OAUTH TOKEN EXCHANGE MODE" section
- docker-compose.yml: drop ENABLE_TOKEN_EXCHANGE/TOKEN_EXCHANGE_CACHE_TTL from
the keycloak service (dead flags)
- docs/webhook-management-guide.md: drop the token-exchange deployment section
- docs/configuration-migration-v2.md: drop the token-exchange migration scenario
- docs/observability.md: drop the never-emitted mcp_oauth_token_exchange_total
Auth ADR status corrections:
- ADR-004: Draft -> Superseded by ADR-022/ADR-023 (token-exchange/federated
design not adopted); note the three supported modes.
- ADR-002: extend the deprecation pointer to ADR-022/ADR-023.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Self-hosting login_flow against Nextcloud's built-in `oidc` app breaks after
~1h when relying on the DCR fallback: the `oidc` app deletes
dynamically-registered clients after `client_expire_time` (default 3600s),
pruning on every /authorize. The MCP server caches the now-deleted client, so
authorize/refresh fail with an "Access forbidden" page permanently — surviving
server restart and connector recreation (issue #907).
- docs/login-flow-v2.md: add "Default IdP setup (Nextcloud oidc app)" with
static-client steps, and a Troubleshooting entry for the #907 symptom/fix;
reframe the OIDC-client env vars as strongly recommended.
- docs/configuration.md: promote NEXTCLOUD_OIDC_CLIENT_ID/_SECRET to strongly
recommended with a DCR-expiry warning; add them to the login_flow example.
- docker-compose.yml: clarify the DCR caveat and point self-hosters to a static
client for login_flow / background sync.
- env.sample.oauth-multi-user: fix the removed `oauth_single_audience` value
(now login_flow) and require a static OIDC client.
- env.sample.oauth-advanced: remove — it configured the removed OAuth
token-exchange mode (no implementation remains; the mode value now errors at
startup). Drop its references in configuration.md / configuration-migration-v2.md.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move 'permissions: contents: read' from workflow level to the record-deployment
job (GitHub Actions least-privilege, rule S8264), keeping this workflow uniform
with the astrolabe copy. Single-job workflow, but consistent and future-proof.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review round 1 follow-ups:
- Reference the built-in $GITHUB_SHA env var in run scripts instead of
interpolating ${{ github.sha }}, removing the GitHub Actions script-injection
surface (SonarCloud security rating on new code).
- Add a concurrency group (cancel-in-progress: false) to
pact-record-deployment.yml so back-to-back tag pushes don't race the recording.
- Add timeout-minutes: 5 to guard against a hung tailnet join.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the missing record-deployment half of the Pact can-i-deploy loop and
stops can-i-deploy from failing every merge while the broker's production
environment is still empty.
- New pact-record-deployment.yml: on tag push, records a production
deployment of nextcloud-mcp-server keyed by the tagged commit SHA, which
matches the SHA pact.yml publishes consumer pacts / verification results
with. Recording the tag string would not link to the verified pacts.
- pact.yml can-i-deploy: wrapped in shadow mode (runs for signal, emits a
warning annotation on failure, always exits 0). can-i-deploy cannot pass
until both nextcloud-mcp-server and astrolabe have recorded a production
deployment, so gating now would block merges on a bootstrap gap.
Tracked on Deck card #325. Promotion to a hard gate is a follow-up.
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>
The inline _process_pdf path does not emit document_escalation_suppressed_total;
the "what-if OCR" counter is instrumented only on the per-tier external path
(evaluate_escalation / _parse_pdf_tier). Comment the inline OCR gate so a reader
doesn't mistake the omission for a bug. Deferred the assert_never nit (typing
.assert_never is 3.11+; Literal+frozen already guard construction) and the
pre-existing minimum-ValueError pass (only "ocr"/None are ever passed).
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>
- escalation: EscalationDecision.kind is now Literal["hop","suppressed"] so ty
catches a bad kind statically (and the processor branch is exhaustive).
- processor: simplify the suppressed-escalation log line (no longer repeats
to_tier / tier).
- registry: clarify _tier_available's ignore_enabled drops the OCR-enabled gate
specifically (a future per-tier gate would extend the condition).
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>
