Round-6 claude-review (ready to merge): the three new GatewayProvider
usage/bearer tests lacked @pytest.mark.unit, so `pytest -m unit` skipped them
even though every other new test in this PR is marked. Add the marker to the
three new tests (leaving the pre-existing unmarked tests in the file alone).
Remaining 🟢 items (OpenAI embed() dual path, recursion-invariant runtime
enforcement, Bedrock sync-in-async) are acknowledged deferrals — separate
refactors, unchanged.
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-3 claude-review finding:
- 🟡 Double _ensure_bearer() on gateway.embed_batch(). Round 1 made
OpenAIProvider.embed_batch() delegate to embed_batch_with_usage(); because
GatewayProvider overrode both embed_batch() and embed_batch_with_usage() (each
calling _ensure_bearer), gateway.embed_batch() refreshed the bearer twice
(the second a cache-hit no-op). Remove the now-redundant embed_batch()
override: OpenAI's embed_batch() routes through embed_batch_with_usage(),
which the gateway still overrides, so the bearer refreshes exactly once on
every path. The remaining two overrides (embed + embed_batch_with_usage) cover
all four entrypoints; documented the topology.
- 🟢 Added test_gateway_embed_batch_ensures_bearer_once locking in the single
refresh.
Cohere token-fallback (🟢 nit) is already covered by
test_bedrock_with_usage_estimates_when_token_count_absent.
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 claude-review findings:
- 🟡 Base-class recursion invariant: documented on embed_with_usage /
embed_batch_with_usage that a provider overriding embed()/embed_batch() to
delegate to the *_with_usage variant MUST also override that variant, or the
two recurse. (No recursion today; the shipped providers pair the overrides.)
- 🟡 Processor metering had no unit test: extracted the two-event recording
into a module-level record_indexing_usage() helper and added
tests/unit/test_processor_metering.py (value mapping, flag/zero-chunk no-ops,
best-effort failure swallowed).
- 🟡 SonarQube hotspots (python:S5332) were 3 http:// URLs in the new test
fixtures (mock hosts, never contacted) blocking the quality gate
(new_security_hotspots_reviewed). Switched them to https:// so no hotspot is
raised.
- 🟢 Zero-chunk guard: record_indexing_usage() no-ops when chunk_count == 0, so
an empty document no longer writes zero-value billing rows.
Deferred (stated on the PR): Mistral x.index-or-0 sort key (pre-existing,
equivalent), CHANGELOG note for the Ollama /api/embed switch (CHANGELOG is
commitizen-generated from commit bodies, which document it), class-var
query_token_count (safe under the per-request instance pattern).
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
embeddings_queries now records the embedding request's token count (the unit
upstream providers bill on) instead of an operation count, and fires on the
indexing path too. Previously only semantic search recorded it (value=1), so a
re-indexing run produced no embeddings_queries events at all — only pages_chunks.
- Provider layer: additive embed_with_usage / embed_batch_with_usage surface the
per-request token count (Mistral/OpenAI usage.total_tokens, Bedrock Titan
inputTextTokenCount, Ollama prompt_eval_count); a char-based estimate is the
fallback (Simple, and any provider/response without a token field). Gateway and
EmbeddingService forward through. The count travels as a return value / a
per-request SearchAlgorithm attribute — never on the singleton — so concurrent
indexing + search can't mis-attribute bills.
- Indexing (vector/processor.py): records embeddings_queries (value=batch tokens)
alongside the existing pages_chunks event.
- Search (server/semantic.py): value is now the query embedding's token count,
relayed from BM25HybridSearchAlgorithm via query_token_count.
The astrolabe_embeddings_queries Stripe meter (sum aggregation) now sums tokens
with no CP/Terraform change. The meter "queries"->tokens naming/unit
clarification (homelab-terraform #254) + CP rollup/portal copy is a follow-up.
Deck #67.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
EMBEDDING_GATEWAY_URL is configured as a bare origin (scheme://host:port) —
the deployment's Service URL. GatewayProvider now appends the gateway's /v1
base path before handing the URL to the OpenAI SDK, so both embed posts
({base}/embeddings) and dimension discovery ({base}/models) land under /v1.
Idempotent: a URL already ending in /v1 is left unchanged.
This lets EMBEDDING_GATEWAY_URL stay a bare domain (matching the gitops
Service URLs) instead of requiring a hand-appended /v1.
Also align the `embedding_gateway_model` field default with _DEFAULTS
("mistral/mistral-embed"). The gateway catalog is provider-namespaced, and
_detect_dimension matches `entry.id == embedding_model`; the stale
un-namespaced default would silently miss the catalog entry and leave the
dimension unresolved (re-triggering the external-mode startup crash).
Tests: bare / trailing-slash / idempotent normalization + a bare-origin
discovery test asserting /v1/models. 16 gateway-provider tests pass;
providers + vector suites green (129 total); ruff clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
External-mode tenant pods CrashLoop at startup: Qdrant collection init calls
get_dimension() before any embed(), but GatewayProvider only learns its
dimension lazily after the first embed, and the gateway model isn't an OpenAI
model so the base class can't know it statically.
- Add GatewayProvider._detect_dimension() — the async startup hook the
vector-sync bootstrap already invokes (vector/qdrant_client.py:
hasattr(provider, "_detect_dimension")) for Ollama. It GETs the gateway's
GET /v1/models and sets _dimension from the entry whose id matches the
configured model. Best-effort: any failure (old gateway, model absent,
network) leaves _dimension unset so the inherited lazy detect-on-first-embed
still applies — never fatal. Presents the M2M bearer when configured.
- Switch the default embedding_gateway_model to the gateway's provider-
namespaced id "mistral/mistral-embed" (the gateway routes on the "/"-prefix
and sends "mistral-embed" upstream); collapse a duplicated config field.
Pairs with astrolabe-cloud-website#229 (gateway /v1/models, namespaced ids).
Tests: discovery sets dim w/o embed, sends bearer, non-fatal on
404/absent/error, skips when already known.
- gateway_client: guard token cache with a lazy anyio.Lock so concurrent
embed calls share one M2M token request instead of racing
- status subscriber: distinguish idle fetch timeouts from real broker
errors (log + 5s backoff) instead of swallowing all and spinning
- nats: warn when the bus URL uses unencrypted transport (non-tls://)
- collection_metadata: accept an optional shared httpx client, make TLS
verify explicit, document the unauthenticated control-plane contract
- replace python -O-stripped asserts with explicit ValueError in the bus
status builder and the api metadata source
- document why the nil-UUID sentinel point can't collide with content ids
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SonarCloud:
- Resolve 6 S5332 hotspots (http→https in test fixture URLs).
- S6418: hoist the unauthenticated AsyncOpenAI placeholder to a named constant
+ NOSONAR (genuine non-secret; gateway ignores it when unauthenticated).
- Fix two reliability bugs: None-index guard in the gateway token-cache test
(S2259) and float `> 0.0` instead of `!= 0.0` in the sentinel test (S1244).
- status.py idle path sleeps 0.1s instead of sleep(0) (S7491); NOSONAR on the
protocol-required async no-await aclose() stubs (S7503).
Claude review:
- Remove three leftover debug print() calls in app.py (logger.info already
covers them).
- payload_backfill: drop parsed_at from the backfilled-keys docstring (it is
per-document state, not a deployment scalar); add a clean 404 precondition
for BasicAuth deployments without an OAuth token verifier.
- status.py: task_status typed TaskStatus | None (drop type: ignore).
- nats.py: TODO to thread etags for file/deck/news; note etag default → None.
- factory: warn on unknown INGEST_BUS_URL scheme; raise ValueError instead of
assert for the external-mode preconditions.
- docs/configuration.md: document the decomposition hook-point env vars + that
nats-py ships core (lazy-imported) and external+bus uses two NATS connections.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the seven §10.2 hook-point modules + five env vars so Astrolabe Cloud can
offload document processing to the external document-processor / embedding
gateway. Purely additive: with every setting unset the server behaves exactly
as today, so self-hosters are unaffected (Deck #92).
Hook points (all default to current monolith behavior):
- config: EMBEDDING_PROVIDER, INGEST_MODE, STATUS_BACKEND,
COLLECTION_METADATA_SOURCE, FACT_EVENT_EMITTER (+ supporting settings),
validated in Settings.__post_init__ (fail-fast STATUS_BACKEND=local with
INGEST_MODE=external); shared canonical.py.
- vector/payload_keys.py + acl_hash.py: cross-impl NAMESPACE/point_id (§2.2)
and BLAKE2b-128 ACL hash (§11), pinned by fixtures shared with the
document-processor repo.
- embedding/gateway_client.py: OpenAI-compatible GatewayProvider authenticating
via M2M OIDC client-credentials (separate realm); manual-only registry entry.
- vector/collection_metadata.py: sentinel-point / API metadata source with env
fallback.
- vector/queue/: hexagonal ingest producer ports + memory/NATS adapters
(Postgres seam); INGEST_MODE=external publishes mcp.ingest.requested.{tenant}
instead of the in-memory stream and skips the in-process processor pool. The
lifespan becomes a composition root across both deployment branches.
- vector/queue/status.py: STATUS_BACKEND=bus subscriber feeding a StatusStore
the vector-sync status endpoint reads.
- admin/payload_backfill.py: POST /api/v1/admin/payload-backfill (admin scope);
processor writes the new payload keys; query-side ACL pre-filter gated behind
ACL_PREFILTER_ENABLED (default off).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
