fix: use provider-backed embeddings for remote agents - #857
Closed
seonghobae wants to merge 177 commits into
Closed
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChanges오케스트레이터 실행과 런타임
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to The current head changes remote embedding execution and the CI test image, but it still carries a likely test-runner startup failure and weakens validation of immutable CI images. Several unresolved runtime, API-contract, accounting, timeout, and reliability issues also remain, so this PR should not merge until the concrete correctness and availability risks are fixed or explicitly accepted. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.95% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 683 functions across 61 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
![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.
…o fix/provider-backed-embedding-batch
seonghobae
enabled auto-merge (squash)
August 26, 2026 03:23
…tch' into fix/provider-backed-embedding-batch # Conflicts: # contextual_orchestrator/cost_router.py # contextual_orchestrator/orchestrator.py # tests/test_cost_router.py # tests/test_kv_credentials.py
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…tch' into fix/provider-backed-embedding-batch
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…tch' into fix/provider-backed-embedding-batch
…tch' into fix/provider-backed-embedding-batch
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ixes Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae
pushed a commit
that referenced
this pull request
Aug 30, 2026
…ixes Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Conflict inventory (fail-closed diagnostic). Exact PR head: |
Contributor
|
Reusable resolver stopped fail-closed. Exact PR head: Executable, structured-data, add/delete, or rename conflicts require semantic resolution.
|
seonghobae
pushed a commit
to ContextualWisdomLab/.github
that referenced
this pull request
Aug 30, 2026
- Clean-merge subtotal said 5 but named 6 PRs; corrected to 6, and added the omitted #1391 to the trivial-conflicts list so 6+7+2=15 reconciles with the stated total. - Compact cross-repo references to the owner/repo#num binding convention: ContextualWisdomLab/contextual-orchestrator#857 and ContextualWisdomLab/LineageWeave#667. Co-Authored-By: Claude <noreply@anthropic.com>
This was referenced Aug 30, 2026
Contributor
Author
|
Status check (2026-08-30, this cycle):
No new code changes from this pass — the two narrowly-safe fixes from the prior cycle (readiness-recovery Generated by Claude Code |
seonghobae
added a commit
to ContextualWisdomLab/.github
that referenced
this pull request
Aug 30, 2026
* docs(gaps): restore section dropped by #1423's merge, correct stale claim PR #1423 merged (ebd1b8b) using a merge-conflict resolution that kept only its own branch's dated section and silently dropped main's already-published '2026-08-30 sidecar pin staleness recurrence' section (the #1422 fix's own record) from the file entirely. That merge also left #1423's own section claiming #1413/#1422 'have not merged' even though both are its own git ancestors. Restore the dropped section verbatim, annotate the stale section as superseded, and append an accurate dated entry recording this cycle's backlog-refresh work: 15 PRs merged/retriggered against the new main, 8 left untouched for real conflicts, one independent sidecar/review- dispatch defect newly observed on #1420, and a Gap ID collision (G-15) renumbered to G-16 on #1348. Verified: git diff against origin/main shows this change is purely additive (no main content removed); pytest tests/test_product_technical_gap_baseline.py tests/test_pr_governance_audit_contract.py (8 passed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw * docs(gaps): fix Devin-flagged count/reference errors in #1424 - Clean-merge subtotal said 5 but named 6 PRs; corrected to 6, and added the omitted #1391 to the trivial-conflicts list so 6+7+2=15 reconciles with the stated total. - Compact cross-repo references to the owner/repo#num binding convention: ContextualWisdomLab/contextual-orchestrator#857 and ContextualWisdomLab/LineageWeave#667. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
This repo-local, PR-scoped workflow (triggered on every push to this branch) has now failed twice on separate audit cycles (2026-08-30 00:xx and 05:38 UTC) with the same outcome: the conflict against main spans 15+ files across core orchestrator modules and is too large/high-risk to auto-resolve safely, so the reusable resolver aborts without pushing a resolution. Per docs/product-goal-directive.md #2 ("Self-modifying/Source-fix Workflow는 목적 달성 후 삭제하고 잔존 시 관찰·제거한다"): a self-modifying workflow that has not achieved its purpose and remains should be removed rather than left to keep re-running and failing on every future push. It is also a repo-local one-off, which is a drift source under this org's "central workflows, not local copies" convention (ContextualWisdomLab/.github's own README/CLAUDE.md). This does not touch the underlying automation/one-shot-pr-conflict-resolver reusable workflow, only this PR's caller. The real DIRTY conflict on this branch is unresolved and still needs manual, file-by-file merge/rebase attention given its size (111 changed files / 13,646 additions against a base ~4 days behind current main) — see the PR comment for repair guidance. Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
Contributor
Author
|
Removed the leftover The underlying blocker is unchanged: GitHub reports this PR Repair path for a human or a dedicated follow-up session: gh pr checkout 857
git fetch origin main
git merge origin/main # or: git rebase origin/main
# resolve conflict markers in the 15 files above
git status --short
git add <resolved files>
git commit # (or, if rebasing: git rebase --continue)
git push origin fix/provider-backed-embedding-batch # add --force-with-lease only if you rebasedSeparately, Generated by Claude Code |
Contributor
|
Scheduled review-feedback autofix for this PR head.
|
Contributor
|
Scheduled review-feedback autofix for this PR head.
|
Contributor
|
Scheduled review-feedback autofix for this PR head.
|
Contributor
Author
|
설계 결정: 추상화는 사용하지 않습니다. 와 는 같은 endpoint를 쓰더라도 서로 다른 모델 catalog를 노출할 수 있는 독립 credential/account boundary이므로 각각 discovery·보존·실패 판정을 수행해야 합니다. 모델 간 동적 선택과 latency/quality evidence 공유는 provider 이름 추정이 아니라 오직 명시적 경계에서만 수행하십시오. 이 PR이 해당 전제를 포함하면 제거하거나 현재 head에서 재검증해 주세요. |
Contributor
Author
|
일반화: 앞 코멘트의 NVIDIA는 예시일 뿐입니다. 모든 KV credential은 독립 provider-account/catalog boundary입니다. 같은 vendor/endpoint/model ID라도 서로 다른 key의 모델·권한·가격·privacy·가용성·실패 상태가 같다고 추정하거나 dedupe하지 마십시오. 개념은 제거하며, 논리적 모델 동등성과 latency/quality evidence 공유는 오직 operator가 명시한 으로만 성립합니다. |
Contributor
Author
|
최종 일반 계약: 모든 KV credential은 독립 provider-account/catalog boundary입니다. 같은 vendor, endpoint, model ID라도 서로 다른 key의 모델, 권한, 가격, privacy, 가용성, 실패 상태를 같다고 추정하거나 dedupe하지 않습니다. provider family는 없으며, 논리적 모델 동등성과 latency/quality evidence 공유는 오직 operator가 명시한 model_group으로만 성립합니다. |
Contributor
Author
|
Propagation update: PR #941 is now merged to main at 42da1d5. This PR branch conflicts with automatic base update, so its implementation must preserve the main contract: every KV credential account is discovered independently; vendor or endpoint identity does not imply model equivalence; only explicit model_group membership shares routing evidence; peak observed RPM and TPM remain measured per account-model route. The protected base already enforces this contract even before this branch resolves its conflicts. |
Contributor
|
Scheduled review-feedback autofix for this PR head.
|
* fix: classify structured synthesis provider failures Signed-off-by: Seongho Bae <me@seonghobae.me> * fix: preserve stacked exhaustion contract Signed-off-by: Seongho Bae <me@seonghobae.me> --------- Signed-off-by: Seongho Bae <me@seonghobae.me>
Contributor
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ Typed synthesis failures lose status
When virtual structured synthesis hits a deadline, terminal tool stop, or malformed response, the catch-all rewrites it to NoViableAgentError. Clients receive 503 instead of 504, 409, or 502.
Contributor
There was a problem hiding this comment.
🔍 Feature lacks research grounding
This new durable provider scheduler includes no paper PDF or citation-and-summary material required by the repository’s research-grounding convention.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+147
to
+155
| self.embedding_batch_backend = ProviderEmbeddingBatchBackend( | ||
| run_provider_embeddings, | ||
| job_registry=registry, | ||
| max_concurrency=getattr(orchestrator.client, "local_concurrency", 1), | ||
| claim_lease_seconds=( | ||
| float(orchestrator.client.timeout) | ||
| if registry.durable and float(getattr(orchestrator.client, "timeout", 0)) > 0 | ||
| else None | ||
| ), |
Contributor
There was a problem hiding this comment.
🔴 Remote sync embeddings always fail
Any remote embedding agent makes embedding_batch_backend asynchronous. Every synchronous embedding request polls once and returns 503 while its work continues.
Prompt for agents
Restore the synchronous /v1/embeddings contract for remote agents while retaining asynchronous behavior for /v1/batch/embeddings. CostRoutingCoordinator currently selects one ProviderEmbeddingBatchBackend for both surfaces, and complete_embeddings_batch performs only one immediate poll. Introduce a clear synchronous provider execution path or an explicit bounded wait used only by the synchronous endpoint. Ensure mixed mock/remote pools still route each selected agent correctly and do not leave paid background work after returning 503.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+804
to
+813
| def poll(self, job: BatchJob) -> Dict[str, Any]: | ||
| """Return queued, running, completed, or failed without blocking.""" | ||
| status = str(self._states.get(job.job_id, "failed")) | ||
| document = { | ||
| "job_id": job.job_id, | ||
| "status": status, | ||
| "is_complete": status in {"completed", "failed", "cancelled"}, | ||
| } | ||
| if status == "failed": | ||
| document["failure"] = dict(self._errors.get(job.job_id, {})) |
Contributor
There was a problem hiding this comment.
🔴 Provider failures become successful batches
A failed provider job sets is_complete true. The coordinator then caches empty embeddings and reports the batch as completed permanently.
Prompt for agents
Propagate terminal provider states through CostRoutingCoordinator.embeddings_batch_document instead of treating every is_complete state as successful. Failed and cancelled jobs must retain their status and structured failure or cancellation details, must not synthesize empty completed embeddings, and must not cache that synthesized success document.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+141
to
+145
| prompt_tokens = sum( | ||
| int(self.token_counter.count_text(request.input_text, request.model)) | ||
| for request in requests | ||
| ) | ||
| return vectors, prompt_tokens |
Contributor
There was a problem hiding this comment.
🔴 Provider usage becomes local estimates
run_provider_embeddings replaces upstream usage with a local token count. Billing records label that estimate measured and can diverge from provider charges.
Prompt for agents
Preserve embedding usage from the provider response through ModelClient.embed, ProviderEmbeddingBatchBackend, and CostRoutingCoordinator. Do not substitute token-counter output for provider-reported counts. If the provider supplies no valid usage, record an explicitly estimated or unavailable measurement rather than measured usage. Define allocation of batch-level provider usage across inputs without changing the reported total.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+714
to
+718
| job_id = f"providerembed_{uuid.uuid4().hex}" | ||
| self._requests[job_id] = list(requests) | ||
| self._deadlines[job_id] = time.time() + self._registry.retention_seconds | ||
| self._states[job_id] = "queued" | ||
| self._terminal_events[job_id] = threading.Event() |
Contributor
There was a problem hiding this comment.
🟥 Embedding batches lack owner isolation
Provider-backed jobs store no principal identity. Any authenticated inference caller with a batch ID can retrieve another caller's embeddings and metadata.
Was this helpful? React with 👍 or 👎 to provide feedback.
Contributor
Author
|
Supersession audit against current Concrete evidence:
Closing this obsolete integration branch in favor of #970. This does not assert that #970 is merge-ready; its own exact-head protected checks and independent approval still govern acceptance. |
Contributor
Author
|
Closed as fully superseded by #970 after exact-head behavioral/file audit. No merge or bypass performed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
29 passedacross credential, batch embedding, routing-boundary, and cost-router testsgit diff --checkThis repairs the runtime boundary needed by LineageWeave Global Ask. It does not bypass protected review or checks.
Summary by CodeRabbit
새 기능
개선 사항
Retry-After를 반환합니다.문서화