add readable multi-branch Qdrant aliases - #243
Conversation
rostilos
commented
Aug 7, 2026
rostilos
commented
- publish atomic aliases for primary and retained branch generations
- preserve immutable generation aliases as the analysis source of truth
- reconcile aliases for existing active indexes after deployment
- serialize updates per branch while allowing different branches to run in parallel
- retain backward-compatible project-level aliases
- update RAG configuration and operator documentation
- add Java and Python coverage for alias publication and reconciliation
- publish atomic aliases for primary and retained branch generations - preserve immutable generation aliases as the analysis source of truth - reconcile aliases for existing active indexes after deployment - serialize updates per branch while allowing different branches to run in parallel - retain backward-compatible project-level aliases - update RAG configuration and operator documentation - add Java and Python coverage for alias publication and reconciliation
|
Important Review skippedToo many files! This PR contains 213 files, which is 113 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (213)
You can disable this status message by setting the 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 |
|
/codecrow analyze |
![codecrow-local[bot]](https://avatars.githubusercontent.com/u/85498741?s=60&v=4){kind=small}
Group only plausible duplicate candidates, preserve uncertain findings, and retain all merged locations. Bind MCP verification to the exact reviewed commit, finding ID, file, and source window while keeping model context token-efficient.
Add bounded archive extraction, case-safe branch aliases, preserve RAG settings through compatibility updates, and clean up all registered branch generations.
|
| Status | PASS WITH WARNINGS |
| Risk Level | HIGH |
| Review Coverage | 95 files analyzed in depth |
| Confidence | HIGH |
Executive Summary
This PR introduces readable multi-branch Qdrant aliases and related branch-aware indexing, analysis coordination, archival, and verification changes across Java and Python services. The overall design is substantial and addresses the intended multi-branch workflow, but the review identified high-risk integration defects involving repository persistence and MCP Stage 3 execution, along with additional runtime, cleanup, and test-build concerns. No task context was provided, so task-coverage confidence is based solely on the changed files and cross-file review.
Recommendation
Decision: PASS WITH WARNINGS
The PR should receive focused follow-up before merge, particularly for the high-severity integration issues and build/runtime paths identified in the detailed findings. The broader implementation can proceed through review, but merging without addressing or explicitly accepting these risks is not recommended.
Issues Overview
| Severity | Count | |
|---|---|---|
| 🔴 High | 2 | Critical issues requiring immediate attention |
| 🟡 Medium | 7 | Issues that should be addressed |
| ✅ Resolved | 5 | Resolved issues |
Analysis completed on 2026-08-10 16:48:04 | View Full Report | Pull Request
📋 Detailed Issues (9)
🔴 High Severity Issues
Id on Platform: 4011
Category: 🐛 Bug Risk
File: .../orchestrator/stage_3_aggregation.py:431
MCP executor call uses unsupported arguments
The changed Stage 3 path constructs McpToolExecutor with review_revision and verification_issues arguments. The resolved current executor declaration accepts only (mcp_client, request, stage), so enabling MCP Stage 3 raises TypeError before verification starts and can fail the review flow instead of producing a report. This is a cross-module API mismatch between the changed call site and python-ecosystem/inference-orchestrator/src/service/review/orchestrator/mcp_tool_executor.py.
💡 Suggested Fix
Update McpToolExecutor.__init__ to accept and use the new verification context, or keep the constructor call compatible and pass that context through a separately supported API. Add an integration test that executes _stage_3_with_mcp with the real constructor signature.
Id on Platform: 4014
Category: 🐛 Bug Risk
File: .../persistence/AnalyzedCommitRepository.java:36
JPQL references missing entity field
The added JPQL predicate uses ac.targetBranch. The visible AnalyzedCommit entity definition lists only id, project, commitHash, analyzedAt, analysisId, and analysisType, with no targetBranch field or accessor (RAG-9f957c9fc710bda3). Unless another unshown entity change adds that mapped property, Spring Data validation will fail when creating this repository, preventing application startup; the corresponding derived method has the same dependency.
💡 Suggested Fix
Add a persisted targetBranch property to AnalyzedCommit with the required schema migration and update its constraints, or remove these branch-scoped repository methods and use fields that actually exist.
🟡 Medium Severity Issues
Id on Platform: 4009
Category: 🐛 Bug Risk
File: .../service/RagOperationsServiceImpl.java:944
Legacy stale indexes are not deleted
Cleanup now routes every stale branch through deleteBranchIndex(project, branch, eventConsumer). That method only deletes collections found through the durable branch-generation registry; when a stale branch exists only in the legacy shared collection returned by getIndexedBranches, it has no registered generations and the legacy collection is never deleted. Cleanup therefore reports the branch as failed and leaves stale vectors behind.
💡 Suggested Fix
Make deleteBranchIndex fall back to deleting the legacy branch collection when no durable generations are registered, or retain the legacy ragPipelineClient.deleteBranch(...) path in cleanup for branches without exact-generation records.
Id on Platform: 4010
Category: 🐛 Bug Risk
File: .../webhookhandler/BitbucketCloudBranchWebhookHandler.java:170
Bitbucket webhook bypasses dependency gating
This handler now calls processAfterDependencyGate, which explicitly skips the processor's compatibility awaitPrAnalysis barrier. Unlike the generic pipeline path, this call does not pass a persisted Job or invoke BranchAnalysisGateService.awaitDependencies in the visible handler. A Bitbucket branch webhook can therefore begin branch reconciliation while an older PR analysis is still active, reintroducing the ordering race that the new durable gate is intended to prevent.
💡 Suggested Fix
Create or resolve the durable branch job before invoking the processor and run awaitDependencies with that job, or keep this handler on process(...) unless an equivalent durable gate has already been executed by the surrounding webhook dispatch path.
Id on Platform: 4012
Category: 🔒 Security
File: .../orchestrator/mcp_tool_executor.py:98
Stage 3 permits unbound file evidence
Stage 3 only uses the verification identity when it happens to resolve to a known issue. If verificationId is missing or unknown, _verification_line_for_path returns zero, no anchor window is applied, and the request still proceeds against the pinned revision. This lets a malformed or untrusted Stage 3 tool call read arbitrary repository content without being bound to one of the reviewed finding locations, weakening the source-evidence contract and allowing unrelated content to influence verification.
💡 Suggested Fix
For Stage 3 getBranchFileContent calls, reject missing or unknown verification IDs before invoking the MCP client. Also validate that the requested file matches the primary or related location associated with that verification record; otherwise return a failed tool result and record invalid evidence.
Id on Platform: 4013
Category: 🐛 Bug Risk
File: .../routers/index.py:152
Streaming endpoint accesses undefined request fields
The streaming endpoint uses direct attribute access for source_tree_sha256 and collection_target. The visible IndexRequest contract in python-ecosystem/rag-pipeline/src/rag_pipeline/api/models.py defines the request with fields such as repo_path, workspace, project, branch, and commit, but does not define either of these attributes (Evidence ID RAG-d51e661f0c42ddcc). Consequently, a normal request entering this endpoint raises AttributeError inside the worker before indexing, and the client receives an SSE error instead of an indexing result. The ordinary endpoint already uses getattr for these compatibility fields, which further indicates that the streaming path is inconsistent.
💡 Suggested Fix
Use getattr(request, "source_tree_sha256", None) and getattr(request, "collection_target", None) in the streaming worker, or add these fields to IndexRequest with the intended validation and defaults. Apply the same compatibility handling to every optional generation field used by this endpoint.
Id on Platform: 4015
Category: 🧹 Code Quality
File: .../service/AnalyzedCommitService.java:65
Branch-scoped receipts violate commit uniqueness
The new branch-aware path deliberately creates a separate receipt with targetBranch, but the visible AnalyzedCommit mapping still has a unique constraint on (project_id, commit_hash) (RAG-9f957c9fc710bda3). When the same commit is analyzed in a second target-branch context, the branch-scoped lookup does not find the first branch's row, then saveAll attempts a duplicate (project, commit) insert and the transaction fails. This defeats the new branch isolation behavior and can prevent successful branch analysis.
💡 Suggested Fix
Change the database uniqueness constraint and corresponding migration to include target_branch (while defining how legacy null-branch rows are handled), or retain project-wide deduplication and avoid inserting branch-specific duplicate receipts.
Id on Platform: 4016
Category: 🧪 Testing
File: .../service/RagBranchIndexStatusServiceTest.java:33
RagConfig test constructor has invalid arity
The added test calls new RagConfig(...) with eight arguments. The visible RagConfig declaration has six record components and no visible eight-argument constructor, so this test cannot compile. That prevents the web-server test source from building.
💡 Suggested Fix
Use the six-component RagConfig constructor or an existing supported overload, and configure any branch-pattern behavior through the corresponding project configuration rather than passing extra arguments.
Id on Platform: 4017
Category: ⚡ Performance
File: .../index_manager/indexer.py:1163
Secondary alias targets are never cleaned up
The changed code swaps every alias in activation_aliases to the new pending collection, but cleanup only examines the primary alias via old_targets.get(alias_name). When publication_aliases contains aliases pointing to prior collections and seal_generation is false, those secondary old collections are no longer reachable through their aliases but are not deleted. Repeated replacements can therefore leave orphaned Qdrant collections and consume storage indefinitely.
💡 Suggested Fix
After a non-sealed replacement, collect the distinct old targets from all aliases, exclude the newly activated collection, and delete each obsolete collection after the swap. Preserve the existing retention behavior when seal_generation is enabled.
Files Affected
- .../service/RagOperationsServiceImpl.java: 1 issue
- .../service/AnalyzedCommitService.java: 1 issue
- .../orchestrator/stage_3_aggregation.py: 1 issue
- .../orchestrator/mcp_tool_executor.py: 1 issue
- .../webhookhandler/BitbucketCloudBranchWebhookHandler.java: 1 issue
- .../index_manager/indexer.py: 1 issue
- .../routers/index.py: 1 issue
- .../persistence/AnalyzedCommitRepository.java: 1 issue
- .../service/RagBranchIndexStatusServiceTest.java: 1 issue
| for (String branch : staleBranches) { | ||
| try { | ||
| boolean success = ragPipelineClient.deleteBranch(workspaceSlug, projectSlug, branch); | ||
| boolean success = deleteBranchIndex(project, branch, eventConsumer); |
There was a problem hiding this comment.
🟡 MEDIUM | Bug Risk
Legacy stale indexes are not deleted
Cleanup now routes every stale branch through deleteBranchIndex(project, branch, eventConsumer). That method only deletes collections found through the durable branch-generation registry; when a stale branch exists only in the legacy shared collection returned by getIndexedBranches, it has no registered generations and the legacy collection is never deleted. Cleanup therefore reports the branch as failed and leaves stale vectors behind.
💡 Suggested fix
Make deleteBranchIndex fall back to deleting the legacy branch collection when no durable generations are registered, or retain the legacy ragPipelineClient.deleteBranch(...) path in cleanup for branches without exact-generation records.
| }; | ||
|
|
||
| Map<String, Object> result = branchAnalysisProcessor.process(request, processorConsumer); | ||
| Map<String, Object> result = branchAnalysisProcessor.processAfterDependencyGate( |
There was a problem hiding this comment.
🟡 MEDIUM | Bug Risk
Bitbucket webhook bypasses dependency gating
This handler now calls processAfterDependencyGate, which explicitly skips the processor's compatibility awaitPrAnalysis barrier. Unlike the generic pipeline path, this call does not pass a persisted Job or invoke BranchAnalysisGateService.awaitDependencies in the visible handler. A Bitbucket branch webhook can therefore begin branch reconciliation while an older PR analysis is still active, reintroducing the ordering race that the new durable gate is intended to prevent.
💡 Suggested fix
Create or resolve the durable branch job before invoking the processor and run awaitDependencies with that job, or keep this handler on process(...) unless an equivalent durable gate has already been executed by the surrounding webhook dispatch path.
| fallback_llm=None, | ||
| ) -> Dict[str, Any]: | ||
| executor = McpToolExecutor(mcp_client, request, stage="stage_3") | ||
| executor = McpToolExecutor( |
There was a problem hiding this comment.
🔴 HIGH | Bug Risk
MCP executor call uses unsupported arguments
The changed Stage 3 path constructs McpToolExecutor with review_revision and verification_issues arguments. The resolved current executor declaration accepts only (mcp_client, request, stage), so enabling MCP Stage 3 raises TypeError before verification starts and can fail the review flow instead of producing a report. This is a cross-module API mismatch between the changed call site and python-ecosystem/inference-orchestrator/src/service/review/orchestrator/mcp_tool_executor.py.
💡 Suggested fix
Update McpToolExecutor.__init__ to accept and use the new verification context, or keep the constructor call compatible and pass that context through a separately supported API. Add an integration test that executes _stage_3_with_mcp with the real constructor signature.
| # Post-review evidence must come from the exact reviewed revision. | ||
| # Never let a model accidentally verify new PR code against target. | ||
| arguments["branch"] = self.review_revision | ||
| verification_id = str( |
There was a problem hiding this comment.
🟡 MEDIUM | Security
Stage 3 permits unbound file evidence
Stage 3 only uses the verification identity when it happens to resolve to a known issue. If verificationId is missing or unknown, _verification_line_for_path returns zero, no anchor window is applied, and the request still proceeds against the pinned revision. This lets a malformed or untrusted Stage 3 tool call read arbitrary repository content without being bound to one of the reviewed finding locations, weakening the source-evidence contract and allowing unrelated content to influence verification.
💡 Suggested fix
For Stage 3 getBranchFileContent calls, reject missing or unknown verification IDs before invoking the MCP client. Also validate that the requested file matches the primary or related location associated with that verification record; otherwise return a failed tool result and record invalid evidence.
| def run_index() -> None: | ||
| try: | ||
| optional_generation_args = {} | ||
| if request.source_tree_sha256: |
There was a problem hiding this comment.
🟡 MEDIUM | Bug Risk
Streaming endpoint accesses undefined request fields
The streaming endpoint uses direct attribute access for source_tree_sha256 and collection_target. The visible IndexRequest contract in python-ecosystem/rag-pipeline/src/rag_pipeline/api/models.py defines the request with fields such as repo_path, workspace, project, branch, and commit, but does not define either of these attributes (Evidence ID RAG-d51e661f0c42ddcc). Consequently, a normal request entering this endpoint raises AttributeError inside the worker before indexing, and the client receives an SSE error instead of an indexing result. The ordinary endpoint already uses getattr for these compatibility fields, which further indicates that the streaming path is inconsistent.
💡 Suggested fix
Use getattr(request, "source_tree_sha256", None) and getattr(request, "collection_target", None) in the streaming worker, or add these fields to IndexRequest with the intended validation and defaults. Apply the same compatibility handling to every optional generation field used by this endpoint.
| @Query(""" | ||
| SELECT ac.commitHash FROM AnalyzedCommit ac | ||
| WHERE ac.project.id = :projectId | ||
| AND ac.targetBranch = :targetBranch |
There was a problem hiding this comment.
🔴 HIGH | Bug Risk
JPQL references missing entity field
The added JPQL predicate uses ac.targetBranch. The visible AnalyzedCommit entity definition lists only id, project, commitHash, analyzedAt, analysisId, and analysisType, with no targetBranch field or accessor (RAG-9f957c9fc710bda3). Unless another unshown entity change adds that mapped property, Spring Data validation will fail when creating this repository, preventing application startup; the corresponding derived method has the same dependency.
💡 Suggested fix
Add a persisted targetBranch property to AnalyzedCommit with the required schema migration and update its constraints, or remove these branch-scoped repository methods and use fields that actually exist.
| toSave.add(new AnalyzedCommit(project, hash, AnalysisType.BRANCH_ANALYSIS)); | ||
| AnalyzedCommit analyzed = new AnalyzedCommit( | ||
| project, hash, AnalysisType.BRANCH_ANALYSIS); | ||
| analyzed.setTargetBranch(targetBranch); |
There was a problem hiding this comment.
🟡 MEDIUM | Code Quality
Branch-scoped receipts violate commit uniqueness
The new branch-aware path deliberately creates a separate receipt with targetBranch, but the visible AnalyzedCommit mapping still has a unique constraint on (project_id, commit_hash) (RAG-9f957c9fc710bda3). When the same commit is analyzed in a second target-branch context, the branch-scoped lookup does not find the first branch's row, then saveAll attempts a duplicate (project, commit) insert and the transaction fails. This defeats the new branch isolation behavior and can prevent successful branch analysis.
💡 Suggested fix
Change the database uniqueness constraint and corresponding migration to include target_branch (while defining how legacy null-branch rows are handled), or retain project-wide deduplication and avoid inserting branch-specific duplicate receipts.
| ReflectionTestUtils.setField(project, "id", 42L); | ||
| ProjectConfig config = new ProjectConfig(); | ||
| config.setRagConfig(new RagConfig( | ||
| true, "master", null, null, true, 30, List.of("develop"), true)); |
There was a problem hiding this comment.
🟡 MEDIUM | Testing
RagConfig test constructor has invalid arity
The added test calls new RagConfig(...) with eight arguments. The visible RagConfig declaration has six record components and no visible eight-argument constructor, so this test cannot compile. That prevents the web-server test source from building.
💡 Suggested fix
Use the six-component RagConfig constructor or an existing supported overload, and configure any branch-pattern behavior through the corresponding project configuration rather than passing extra arguments.
| raise | ||
|
|
||
| if old_target and old_target != pending_collection_name: | ||
| old_target = old_targets.get(alias_name) |
There was a problem hiding this comment.
🟡 MEDIUM | Performance
Secondary alias targets are never cleaned up
The changed code swaps every alias in activation_aliases to the new pending collection, but cleanup only examines the primary alias via old_targets.get(alias_name). When publication_aliases contains aliases pointing to prior collections and seal_generation is false, those secondary old collections are no longer reachable through their aliases but are not deleted. Repeated replacements can therefore leave orphaned Qdrant collections and consume storage indefinitely.
💡 Suggested fix
After a non-sealed replacement, collect the distinct old targets from all aliases, exclude the newly activated collection, and delete each obsolete collection after the swap. Preserve the existing retention behavior when seal_generation is enabled.
1 participant
