feat: add Azure embedding client and semantic index utils (LLMO-7445)

[tb-adbe]

What

Shared building blocks (step 2 of 4) for the Lookup Service topic dimension — semantic opportunity-by-topic lookup (LLMO-7445, M2 v1). Two packages:

spacecat-shared-gpt-client

• AzureEmbeddingClient — createFrom(context) (reads AZURE_EMBEDDING_*, falling back to AZURE_OPENAI_* for endpoint/key/api-version) and createEmbeddings(inputs, { dimensions? }) → number[][]. Separate from the chat AzureOpenAIClient.
• EmbeddingProvider typedef so consumers depend on the interface, not the concrete client — the model/provider is swappable later via a coordinated re-embed with no code-shape change.
• Exported from the package root (index.js + .d.ts).

spacecat-shared-data-access

• semantic-index.utils.js mirroring url-index.utils.js:
  • syncOpportunitySemantic — full-replace an opportunity's per-source vectors scoped to (entity_id, source_type) (upsert + prune), best-effort.
  • copyEntityVectors — INSERT … SELECT copy of vector rows to a new entity id (history snapshot, no re-embed).
  • lookupOpportunitiesByVector — the ANN reader (site-scoped, over-fetch + dedup to distinct opportunities).
  • getQueryEmbedding / upsertQueryEmbedding / touchQueryEmbedding — semantic_query_embedding cache helpers.
  • serializeVector, SEMANTIC_INDEX_TABLES allow-list.

Contract

Both the audit-worker write path and the api-service read path embed via the same AzureEmbeddingClient / text-embedding-3-small (1536 dims, cosine), so the two share one vector space by construction.

Notes

• 100% / 97%-branch coverage on new files (package requirement); unit tests included for both.
• Merge → semantic-release auto-publishes both packages (a feat: minor bump); consumers (audit-worker, api-service) bump the dependency version after the release lands.
• Depends on the data-service schema (step 1) at runtime, not at build time.

Related Issues

• Jira: LLMO-7445
• Part of the Lookup Service topic-dimension chain: adobe/mysticat-data-service#1064 (step 1) → this PR (step 2) → feat: index offsite opportunity topics for semantic lookup (LLMO-7445) spacecat-audit-worker#2982 (step 3) → feat: add opportunities by-topics semantic lookup endpoint (LLMO-7445) spacecat-api-service#3298 (step 4)

[MysticatBot]

Hey @tb-adbe,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Request changes - four must-fix issues around input validation, data integrity, and credential hygiene.
Complexity: HIGH - large diff (1431 lines, 8 files).
Changes: Adds an Azure OpenAI embedding client and semantic vector storage/retrieval utilities (sync, copy, ANN lookup, query-embedding cache) with tests for both (8 files).

Must fix before merge

1. [Important] dims not validated in query-embedding cache functions - silent cache-key corruption - semantic-index.utils.js:370 (details inline)
2. [Important] parseVector silently returns NaN on malformed vectors - data integrity gap - semantic-index.utils.js:102 (details inline)
3. [Important] config (with API key) exposed as public instance property - azure-embedding-client.js:100 (details inline)
4. [Important] No retry/backoff for transient Azure API failures (429/5xx) - azure-embedding-client.js:142 (details inline)

Non-blocking (3): minor issues and suggestions

• nit: normalizeText silently coerces non-string input to '' instead of throwing, creating a latent collision surface for future callers that skip the pre-check - semantic-index.utils.js:73
• nit: syncOpportunitySemantic hardcodes the table name while copyEntityVectors accepts it as a parameter; asymmetry will confuse when SEMANTIC_INDEX_TABLES grows - semantic-index.utils.js:239
• suggestion: copyEntityVectors materializes all source rows into JS memory rather than using a server-side RPC; works at current cardinalities but worth a follow-up for an rpc_copy_entity_vectors function - semantic-index.utils.js:277

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 8m 25s | Cost: $7.63 | Commit: 94c48fcc0adee9c773dc95ea5af3965828c241df
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot]

issue (blocking): dims is not validated in getQueryEmbedding, upsertQueryEmbedding, or touchQueryEmbedding. The cache key is (text_hash, model, dims), but while model gets assertId validation, dims is passed to .eq('dims', dims) unchecked. Passing undefined or a non-number produces a silent cache miss (redundant embedding API calls) or upserts a row with null dims that can never be found again.

Fix: Add if (typeof dims !== 'number' || !Number.isFinite(dims)) throw new ValidationError('dims is required') at the top of each function, or create an assertDims helper matching the assertId pattern.

[MysticatBot]

issue (blocking): inner.split(',').map(Number) returns NaN elements for non-numeric segments (e.g., corrupted DB value [0.1,abc,0.3] produces [0.1, NaN, 0.3]). The downstream serializeVector rejects NaN via Number.isFinite, but the error surfaces far from the root cause with a misleading message.

Fix: Validate after parsing:

const nums = inner.split(',').map(Number);
return nums.some(Number.isNaN) ? null : nums;

This returns null (cache-miss semantics) for corrupt data, matching the function's existing null-return convention.

[MysticatBot]

issue (blocking): this.config (containing apiKey) is a public property. Any code with a reference to the client can read client.config.apiKey. The class already uses private methods (#post, #logDuration), so #config is consistent with the existing pattern.

Fix: Store as #config private field. Adjust the test to verify behavior (successful embedding call with expected inputs) rather than inspecting client.config.apiKey directly.

[MysticatBot]

issue (blocking): No retry/backoff for transient failures. Azure OpenAI returns 429 (rate limit) with a Retry-After header under normal operation, and 5xx responses are transient. A single failure here fails the upstream sync operation entirely. Both the audit-worker write path and api-service read path will depend on this client.

Fix: Add exponential backoff with jitter for 429/5xx (respecting Retry-After), or accept a maxRetries option so callers can opt in. If intentionally deferred to step 3, document that in a code comment.

[github-actions]

This PR will trigger a minor release when merged.

[tb-adbe]

Thanks for the review @MysticatBot — addressed in 0bca592. Summary of each point:

Must-fix

1. dims not validated — added an assertDims guard (finite positive integer) to getQueryEmbedding / upsertQueryEmbedding / touchQueryEmbedding, so a bad dims fails fast instead of silently corrupting the cache key.
2. parseVector NaN — now returns null on any non-numeric segment (cache-miss semantics), and getQueryEmbedding treats a null parse as a miss so the caller re-embeds instead of receiving { vector: null }.
3. config exposed — moved to a private #config field; the two tests that inspected client.config.* are rewritten as behavior checks (and one asserts client.config === undefined).
4. No retry/backoff — added bounded retry for transient 429/5xx: honors a numeric Retry-After, otherwise exponential backoff with jitter. maxRetries defaults to 3 (opt-out via 0, also read from AZURE_EMBEDDING_MAX_RETRIES); non-retryable 4xx still throw immediately.

Non-blocking
5. normalizeText coerces to '' — kept intentionally: it's the guard that toRows (pre-checks type) and the cache helpers ('' → throws "text is required") rely on; throwing here would break toRows.
6. sync hardcodes table vs copy parametrizes it — resolved together with #7: copyEntityVectors is no longer generic-over-table, so all the helpers are now uniformly opportunity-scoped.
7. copyEntityVectors materializes rows in JS — done now rather than deferred. It's a thin wrapper over a new wrpc_copy_opportunity_semantic_vectors write RPC (INSERT … SELECT … ON CONFLICT DO NOTHING, server-side, SECURITY DEFINER) added on the mysticat-data-service branch — no rows round-trip through the caller. (Consumer in spacecat-audit-worker updated to match.)

Also fixed a stale doc reference (vectors are produced by the audit-worker, not Mystique). Coverage stays 100% lines/statements; branches above the 97% floor.

[MysticatBot]

Hey @tb-adbe,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Request changes - one must-fix item remains around retry delay bounds.
Complexity: HIGH - large diff (1572 lines, 8 files).
Changes: Adds an Azure OpenAI embedding client with retry/backoff and semantic vector storage/retrieval utilities (sync, copy via server-side RPC, ANN lookup, query-embedding cache) with tests for both (8 files).
Note: CI checks are currently pending - verify before merge.

Must fix before merge

1. [Important] Unbounded Retry-After delay can stall caller indefinitely in time-bounded execution - azure-embedding-client.js:51 (details inline)

Non-blocking (3): minor issues and suggestions

• nit: AZURE_EMBEDDING_MAX_RETRIES accepts negative integers (e.g. "-5"), which silently behaves as maxRetries=0 - clamp to Math.max(0, ...) for clarity - azure-embedding-client.js:105
• suggestion: touchQueryEmbedding re-derives normalized text + SHA-256 hash that the caller already has from the preceding getQueryEmbedding hit - accepting textHash directly would save redundant work on the hot read path - semantic-index.utils.js:451
• nit: COPY_VECTORS_RPC is exported in index.js but never asserted in the test file unlike the other constants - semantic-index.utils.test.js

Previously flagged, now resolved

• dims validation: assertDims guard added to all three cache functions, rejects non-positive-integer values
• parseVector NaN: returns null for corrupt segments, getQueryEmbedding treats null parse as cache miss
• config exposure: API key moved to private #config field, tests verify client.config === undefined
• No retry/backoff: bounded retry with Retry-After header support, exponential backoff with jitter, configurable maxRetries

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 4m 12s | Cost: $6.98 | Commit: 0bca59265febde9c4adcf20ca09e944201ee1109
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot]

issue (blocking): The Retry-After header value is trusted without an upper bound. A misbehaving upstream, CDN, or Azure outage returning Retry-After: 3600 would block the caller for an hour. In a Lambda/worker with a finite execution budget, this wastes the invocation slot and delays the timeout signal to the orchestrator. The exponential backoff path is naturally bounded by maxRetries * (2^attempt * base), but the Retry-After path is not.

Fix: Cap the honored value, e.g.:

const MAX_RETRY_DELAY_MS = 30_000;
return Math.min(retryAfter * 1000, MAX_RETRY_DELAY_MS);

Alternatively, make the cap configurable alongside retryBaseDelayMs. If the server asks for more than the cap, use the cap - a bounded retry is always preferable to an unbounded sleep in a time-constrained execution context.

[tb-adbe]

Thanks for the follow-up @MysticatBot — addressed in 7d3b022.

Must-fix

1. Unbounded Retry-After — every retry sleep is now capped at retryMaxDelayMs (default 30s, configurable), applied to both the Retry-After and the exponential-backoff paths. A hostile Retry-After: 3600 can no longer stall the invocation past the cap.

Nits

• Negative maxRetries — clamped with Math.max(0, …) at point-of-use, so a negative value (from env or direct construction) behaves as 0 explicitly.
• touchQueryEmbedding re-hashing — the helper now accepts an optional textHash; when the caller passes the hash it already got from the getQueryEmbedding hit, it skips re-normalizing + re-hashing (falls back to deriving when omitted, so it stays backward-compatible). The by-topics read path is updated to thread it through in the sibling api-service PR (#3298).
• COPY_VECTORS_RPC untested — added to the constants assertion.

Coverage stays 100% lines/statements; branches above the 97% floor.

[MysticatBot]

Hey @tb-adbe,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Approve - all prior blocking findings addressed, no new issues.
Complexity: HIGH - large diff (1572 lines, 8 files).
Changes: Adds an Azure OpenAI embedding client with bounded retry/backoff and semantic vector storage/retrieval utilities (sync, copy via server-side RPC, ANN lookup, query-embedding cache) with tests for both (8 files).
Note: CI checks are currently pending - verify before merge.

Non-blocking (2): minor issues and suggestions

• suggestion: retryMaxDelayMs is not clamped unlike maxRetries (which uses Math.max(0, ...)); a zero or negative value silently disables backoff. Adding Math.max(0, ...) or a minimum floor would make the two config paths consistent - azure-embedding-client.js:165
• suggestion: touchQueryEmbedding accepts any non-empty string as textHash without validating the expected SHA-256 hex format (/^[0-9a-f]{64}$/); PostgREST parameterizes the query so there is no injection risk, but format validation would make the contract explicit - semantic-index.utils.js:460

Previously flagged, now resolved

• Unbounded Retry-After delay: every retry sleep now capped at retryMaxDelayMs (default 30s), applied to both Retry-After and backoff paths
• Negative maxRetries: clamped with Math.max(0, ...) at point-of-use
• touchQueryEmbedding redundant re-hashing: accepts optional textHash param to skip re-normalize + re-hash on the hot read path
• COPY_VECTORS_RPC untested: added to the constants assertion

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 17s | Cost: $4.24 | Commit: 7d3b02219cace8cb9b0e5e45785ac4828a27717c
If this code review was useful, please react with 👍. Otherwise, react with 👎.}