Delegate embedding model discovery to contextual-orchestrator - #322
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@opencode-agent review exact current HEAD a6a1d8f. Review the orchestrator-owned embedding model provenance boundary, automatic completeness semantics, and the stacked dependency contract; publish a formal review. |
|
@opencode-agent correction and supersession: the exact current HEAD is |
|
@opencode-agent supersede prior requests: review exact current HEAD |
|
Supersede prior review targets: current exact HEAD is |
|
Review exact current HEAD 7b71045. The orchestrator-owned embedding/provenance boundary and selector isolation are checked on this exact head; required Full test suite and Frontend lint, test, build checks are green. Please publish a formal independent review for this SHA before any merge decision. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@opencode-agent review the exact current head 3b2fdcf. The only delta after the previously reviewed functional head updates the immutable contextual-orchestrator pin to #791 merged head 4c31c3549537ba4813fe25e2bf4efc3e5b56aa3f; upstream runtime source is identical and only ADR numbering changed. Pin tests and exact archive image build pass. |
|
Validation checkpoint: exact current HEAD is |
|
@opencode-agent Please review exact head |
…ty' into feat/orchestrator-owned-embedding-consumer
|
Reviewed and repaired against exact current head
The PR remains blocked pending independent formal review and terminal Checks; no bypass used. |
…ty' into feat/orchestrator-owned-embedding-consumer
|
Exact-head update: current stale-summary base conflict was resolved while retaining orchestrator integration head |
|
The failed Strix artifact was infrastructure-only: no vulnerability report was produced; the run failed before analysis because Strix could not bootstrap Caido ( |
seonghobae
added
the
enhancement
ChatGPT Codex Connector
Current-head review evidence at
|
There was a problem hiding this comment.
📝 Info: Backward-compat client lacks resolved_model_code and embed_many
OpenAiCompatibleEmbeddingClient (embedding_client.py) still takes model: str and only wraps embed; it exposes neither embed_many nor resolved_model_code. If it were ever used with persist_post_content in auto mode (embedding_model_code=None), getattr(embedding_client, 'resolved_model_code', None) would be None and every embedding would be discarded. No runtime path constructs this class (only ContextualOrchestratorEmbeddingClient is built via orchestrator_embedding_client), so this is currently harmless, but the compat wrapper is now inconsistent with the new provenance flow.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| ( | ||
| nullif($2::text, '') is null | ||
| and ( | ||
| not exists( | ||
| select 1 | ||
| from post_content_unit unit | ||
| left join post_content_embedding embedding | ||
| on embedding.post_content_unit_id = unit.post_content_unit_id | ||
| where unit.post_id = $1 | ||
| and embedding.post_content_embedding_id is null | ||
| ) | ||
| and not exists( | ||
| select 1 | ||
| from post_content_unit unit | ||
| join post_content_image image | ||
| on image.post_content_unit_id = unit.post_content_unit_id | ||
| join post_content_image_region region | ||
| on region.post_content_image_id = image.post_content_image_id | ||
| left join post_content_image_region_embedding embedding | ||
| on embedding.post_content_image_region_id = region.post_content_image_region_id | ||
| where unit.post_id = $1 | ||
| and region.description_status_code = 'described' | ||
| and embedding.post_content_image_region_embedding_id is null | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🔍 Automatic completeness requires an embedding for every unit, including text-less image units
In automatic mode (embedding_model_code empty/None, the only runtime path now), post_content_is_complete requires that every post_content_unit has a post_content_embedding (the not exists(... embedding.post_content_embedding_id is null) clause at post_content_queue.py). Image units whose rendered unit_text is empty are never added to embeddable in post_content_persistence.py, so they get no unit embedding, which would keep such a post permanently incomplete and drive the worker to FAILED after retries. This behavior is not newly introduced (the prior exact-model branch imposed the same per-unit requirement while the runtime supplied a configured model), so it is not a regression from this PR — but it is worth confirming that all embeddable units actually receive embeddings under the pinned orchestrator, otherwise ingestion fails closed indefinitely.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ) | ||
|
|
||
| vectors: dict[str, list[float]] = {} | ||
| if embedding_client is not None and embedding_client.available and embedding_model_code: | ||
| vectors: dict[str, tuple[list[float], str | None]] = {} | ||
| if embedding_client is not None and embedding_client.available: | ||
| embeddable = [ | ||
| (f"unit:{chunk.index}", unit_text) | ||
| for chunk, unit_text, _style in prepared |
There was a problem hiding this comment.
🔍 Valid embeddings are discarded when orchestrator omits the model code
In auto mode (embedding_model_code=None), persist_post_content records the batch model code from embedding_client.resolved_model_code, falling back to embedding_model_code (post_content_persistence.py). If the orchestrator returns valid vectors but no model field, resolved_model_code stays None, so the tuple's model code is None and both the region loop (:292) and unit loop (:314) skip persistence entirely — the computed vectors are thrown away and the job stays incomplete for retry. This is the intended fail-closed contract per ADR 0118 point 3 (no provenance-less evidence), and it depends on the upstream #789 contract always returning the resolved model. Flagging so reviewers confirm the orchestrator pin actually satisfies that contract; otherwise embedding ingestion silently never completes.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| ) | ||
|
|
||
| vectors: dict[str, list[float]] = {} | ||
| if embedding_client is not None and embedding_client.available and embedding_model_code: | ||
| vectors: dict[str, tuple[list[float], str | None]] = {} |
There was a problem hiding this comment.
📝 Info: Resolved model code takes precedence over an explicitly-passed legacy model code
In post_content_persistence.py, batch_model_code prefers the client's resolved_model_code over the explicitly-passed embedding_model_code. If a lower-level caller ever passed an explicit legacy model code (e.g. text-embedding-3-large) while using the real ContextualOrchestratorEmbeddingClient, and the orchestrator returned a different/canonicalized model string, the embedding would be persisted under the resolved code. A subsequent completeness check using the explicit-model branch (embedding_model_code = $2) would then never match, leaving the post incomplete forever. This does NOT occur in practice: every runtime caller now passes None/"" (automatic mode, which ignores model code in completeness), and the only callers passing an explicit model are tests whose fake clients lack resolved_model_code, so batch_model_code falls back to the passed value. Noted only because the precedence ordering could bite if a real explicit-model caller is reintroduced.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ) | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🔍 Completeness semantics for empty/absent embedding model changed from 'skip' to 'require'
Previously post_content_is_complete used $2 = '' or (...), so an empty/unset embedding model made the embedding requirement TRUE (embeddings not required for completeness). The new code at post_content_queue.py routes both '' and NULL (via nullif($2::text,'')) into the auto branch, which REQUIRES an embedding for every unit and every described image region regardless of model code. This is the intended ADR 0118 behavior, but it has an operational implication: a deployment that previously ran without an embedding agent (no LLM_GATEWAY_EMBEDDING_MODEL) had post-content jobs succeed with no embeddings; after this change those posts are considered incomplete. When a wake-up event re-claims such a SUCCEEDED job (post_content_worker.py:106-114), it will reprocess and, if the orchestrator has no eligible embedding agent, drive the job to FAILED after the retry limit. This fail-closed outcome is consistent with ADR 0118, but worth confirming that all target deployments do configure an orchestrator embedding agent.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| def embed_many(self, texts: list[str]) -> list[list[float]]: | ||
| """Return embeddings for the supplied texts.""" | ||
| self.resolved_model_code = self._model | ||
| if not texts: | ||
| return [] | ||
| headers = {"authorization": f"Bearer {self._api_key}"} | ||
| payload: dict[str, object] = { | ||
| "inputs": texts, | ||
| "endpoint": "/v1/embeddings", | ||
| "metadata": {"service": "lineageweave", "channel": "post_content_embedding"}, | ||
| } | ||
| if self._model: | ||
| payload["model"] = self._model |
There was a problem hiding this comment.
📝 Info: resolved_model_code reset prevents stale provenance across reused clients
embed_many resets self.resolved_model_code = self._model at the top of each call (embedding_client.py) before any early return. In post_content_persistence.py:189-191 the per-batch model code is read immediately after the awaited embed_many, so provenance is captured per batch. Because the scripts (backfill_post_content.py, import_postgresql_posts.py) reuse a single embedding client across many posts, this reset is what prevents a previous post's resolved model from leaking into a later post whose response omits model (covered by test_orchestrator_embedding_client_does_not_reuse_previous_model_provenance). When the reset falls back to None/empty, the vector is stored with a null model and skipped at insert time (post_content_persistence.py:290-314), keeping the job incomplete for retry — the intended fail-closed path.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def orchestrator_embedding_client(base_url: str, api_key: str, model: str | None = None): | ||
| """Build the batch embedding channel, or the unavailable null client.""" | ||
| if not (base_url and api_key and model): | ||
| if not (base_url and api_key): | ||
| return NullEmbeddingClient() | ||
| return ContextualOrchestratorEmbeddingClient(base_url, api_key, model) |
There was a problem hiding this comment.
📝 Info: orchestrator_embedding_client no longer gates on model presence
orchestrator_embedding_client changed its availability gate from if not (base_url and api_key and model) to if not (base_url and api_key) (embedding_client.py). As a result, callers that pass an empty/None model now get a live ContextualOrchestratorEmbeddingClient (available=True) instead of a NullEmbeddingClient. I confirmed every runtime caller (backend/app/main.py:394, the scripts) either passes real base/key or explicitly checks .available and raises, so the gating now correctly depends only on the orchestrator boundary credentials. This matches the PR intent (orchestrator owns model discovery); noting it because it is a semantic contract change to a shared factory whose future callers will get a live client where they previously got Null.
Was this helpful? React with 👍 or 👎 to provide feedback.
| (index, await asyncio.to_thread(embedding_client.embed, text)) | ||
| for index, text in batch | ||
| ] | ||
| batch_model_code = getattr(embedding_client, "resolved_model_code", None) | ||
| if not isinstance(batch_model_code, str) or not batch_model_code.strip(): | ||
| batch_model_code = embedding_model_code | ||
| for embedding_key, vector in candidates: | ||
| if isinstance(vector, list) and vector and all( | ||
| isinstance(value, (int, float)) and math.isfinite(float(value)) | ||
| for value in vector | ||
| ): | ||
| vectors[embedding_key] = [float(value) for value in vector] | ||
| vectors[embedding_key] = ( | ||
| [float(value) for value in vector], | ||
| batch_model_code.strip() if isinstance(batch_model_code, str) else None, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Per-batch resolved model code relies on shared mutable client state
persist_post_content reads getattr(embedding_client, "resolved_model_code", None) (post_content_persistence.py) immediately after each embed_many/embed call. This depends on the client instance attribute reflecting only the just-completed batch. ContextualOrchestratorEmbeddingClient.embed_many resets resolved_model_code = self._model at entry and updates it from each response, and batches are awaited sequentially, so the read is correct here. This is safe only because a fresh embedding client is built per job (embedding_factory() in the worker) and batches never run concurrently against a shared client; any future concurrent reuse of one client instance would make this provenance read race-prone.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| complete = await post_content_is_complete( | ||
| connection, | ||
| post_id, | ||
| embedding_model_code=model, | ||
| embedding_model_code=None, | ||
| require_structure=require_structure, | ||
| ) |
There was a problem hiding this comment.
📝 Info: queue_post_content_backfill SQL bind renumbering verified
The pre-filter query dropped the embedding_model_code = $1 join predicates and renumbered the remaining binds so $1 is the structure boolean and $2 is the limit, matching the argument order require_structure, limit at queue_post_content_backfill.py. The new static contract test (test_static_sql_review_contracts.py) locks in or ($1::boolean and exists ( and limit $2::bigint. This is consistent and the removed model filter only widens the candidate set, with post_content_is_complete remaining the authoritative gate.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Addressed the remaining exact-head embedding contract and CI issue on
Verification on this exact local head: |
| ) | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
📝 Info: Completeness now requires embeddings regardless of model
The old predicate treated a blank model as complete ($2 = '' or ...). The new null branch instead requires an embedding for every unit and every described region. This matches the fail-closed persistence path and the candidate query in queue_post_content_backfill.py, which also dropped its model filter. Existing rows embedded under an old model code still satisfy the null branch since any embedding counts. The non-null branch stays equivalent to the original.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| def embed_many(self, texts: list[str]) -> list[list[float]]: | ||
| """Return embeddings for the supplied texts.""" | ||
| self.resolved_model_code = self._model |
There was a problem hiding this comment.
📝 Info: Per-batch model provenance reset verified
embed_many resets resolved_model_code at entry, and _record_resolved_model only overwrites on a non-empty response model. A later batch whose response omits model therefore resets provenance to None rather than reusing the prior batch's model, which persistence treats as fail-closed. A fresh client is built per job, so no cross-job leak occurs.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if column is None: | ||
| return default | ||
| if column not in row.keys(): | ||
| if column not in row: |
There was a problem hiding this comment.
📝 Info: Record membership relies on key-based __contains__
_value changed from column not in row.keys() to column not in row. At runtime this runs against asyncpg.Record, whose __contains__ tests column names, so semantics are preserved; tests only exercise dict rows. If that assumption failed the importer would raise spurious KeyError on mapped columns.
Was this helpful? React with 👍 or 👎 to provide feedback.
552af1f
into
fix/stale-summary-buyer-continuity
1 participant
Summary
LLM_GATEWAY_EMBEDDING_MODELandkeep that selector out of the backend Compose environment.
modelfor automatic embedding discovery throughcontextual-orchestrator.
provenance, resetting the resolved value for every batch so stale provenance
cannot leak across documents.
exact legacy matching for explicit lower-level callers.
4c31c3549537ba4813fe25e2bf4efc3e5b56aa3f.fragments.
Dependency and exact-head verification
a903fffe6042fd9d64d233f6888b75c25860810f.f316e0ab5507b101744e632d3c1596a3ed674d22(ordinary two-parent restack).737 passed, 16 skipped, 4 warnings.25 passed.134tests, production build, Storybook,actionlint, andgit diff --checkpassed.were not persisted.
GitHub checks and an independent exact-head approval remain required before
merge.