Conversation
- add reusable public-share links backed by opaque hashed credentials - extract structured test cases from generated QA documentation - repair generated documents that omit marked test scenarios - preserve full QA documents while sharing test-case preview links in Jira - render public preview URLs as Jira links and keep failed handoffs retryable - reconcile interrupted RAG jobs, project states, and revision locks - isolate recovery scheduling from optional alias reconciliation
- preserve all Jira QA sections while replacing only test scenarios with a share link - expose sanitized QA content after public-token validation - redirect authorized users to the project QA document - split environment notes into a separate response section - prevent N/A QA guide titles
add readable multi-branch Qdrant aliases
- make stale lock and RAG recovery concurrency-safe - reuse persisted QA documents when preview or Jira handoff fails - validate marked test cases and trim Jira URL delimiters correctly - align public QA share contracts and include the frontend preview route
- prevent stale RAG jobs from overwriting newer indexing status - persist exact lock ownership for safe recovery and parallel branch indexing - regenerate QA documentation when a pending handoff belongs to another task - add migration and regression coverage
Feature: Public share links
|
Important Review skippedToo many files! This PR contains 139 files, which is 39 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 (139)
You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request adds structured QA-document parsing and public sharing across Python and Java services. It also adds RAG job ownership and recovery coordination, Jira URL-link rendering, scheduler configuration, Maven module wiring, and a frontend submodule update. ChangesQA document generation and public sharing
RAG lifecycle coordination
Jira URL rendering
Frontend revision pointer
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant QAOrchestrator
participant QaAutoDocListener
participant QaDocPublicPreviewService
participant PublicShareLinkService
participant JiraCloudClient
QAOrchestrator->>QaAutoDocListener: return QA document
QaAutoDocListener->>QaDocPublicPreviewService: create preview URL and task comment
QaDocPublicPreviewService->>PublicShareLinkService: issue document share
PublicShareLinkService-->>QaDocPublicPreviewService: frontend share URL
QaDocPublicPreviewService-->>QaAutoDocListener: preview-link comment
QaAutoDocListener->>JiraCloudClient: post task comment
JiraCloudClient-->>JiraCloudClient: render URL as ADF link
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![codecrow-local[bot]](https://avatars.githubusercontent.com/u/85498741?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/RagIndexTrackingService.java (1)
38-63: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSerialize project-level start transitions before assigning ownership.
findByProjectIdForUpdateonly locks an existing status row. Two starts can both observe no row and race into the unique constraint. A second start can also overwrite an active owner because both methods assignactiveJobIdwithout checking the current live state. Branch-scoped analysis locks do not prevent this for different branches of the same project.
java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/RagIndexTrackingService.java#L38-L63: serialize creation and reject a live status owned by a different job.java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/RagIndexTrackingService.java#L191-L201: apply the same ownership guard before changing toUPDATING.java-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/service/RagIndexTrackingServiceTest.java#L89-L130: add coverage for a conflicting active owner and concurrent first-status creation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/RagIndexTrackingService.java` around lines 38 - 63, Serialize project-level start transitions in RagIndexTrackingService.markIndexingStarted before creating or updating the status, preventing concurrent first-status creation and rejecting a live status owned by a different activeJobId. Apply the same ownership guard in the UPDATING transition around the sibling service site. Add tests in RagIndexTrackingServiceTest covering conflicting active ownership and concurrent creation of the first project status; update the listed service sites and test range accordingly.
🧹 Nitpick comments (9)
java-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/service/RagOperationsServiceImplTest.java (1)
805-805: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStub the job ID so the assertion proves ownership propagation.
mockJob.getId()is not stubbed, so the expected0Lis the Mockito default for a primitive return. The assertion passes even if the production code passes a different unstubbed job. Stub a distinct ID and verify that value.♻️ Proposed change
- verify(ragIndexTrackingService).markUpdatingCompleted( - testProject, "main", "current-head", 0, 0, null, 0L); + verify(ragIndexTrackingService).markUpdatingCompleted( + testProject, "main", "current-head", 0, 0, null, 4242L);Add the stub next to the other
mockJobsetup:when(mockJob.getId()).thenReturn(4242L);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/service/RagOperationsServiceImplTest.java` at line 805, Update the mockJob setup in RagOperationsServiceImplTest to stub getId() with a distinct value such as 4242L, then change the expected job ID in the assertion to that value so the test verifies ownership propagation.java-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/service/RagBranchIndexRegistryServiceTest.java (1)
257-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the positive case for
failIfAbandoned.This test covers only the rejection path. No test asserts that a stale operation returns
trueand transitions the operation and its generation toFAILED. Add a case withupdatedAtolder than the cutoff. Also add a case where the status is alreadySUCCEEDEDto confirm the method returnsfalse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/service/RagBranchIndexRegistryServiceTest.java` around lines 257 - 273, Extend the failIfAbandoned tests around abandonmentClaimRechecksAHeartbeatUnderTheOperationLock with a stale operation case that verifies true is returned and both the operation and generation transition to FAILED, plus a SUCCEEDED-status case verifying false is returned. Reuse the existing repository stubbing and assertions for failIfAbandoned, and keep the current heartbeat rejection test unchanged.java-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/branch/RagIndexOperationRecoveryServiceTest.java (1)
24-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
@ExtendWith(MockitoExtension.class)for consistency.The sibling test
RagBranchIndexRegistryServiceTestuses the Mockito extension with@Mockfields andlenient()where needed. This class creates mocks manually, so strict stubbing does not apply and unused stubs stay undetected. Aligning the two classes would have surfaced the missingfindByIdstub noted above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/branch/RagIndexOperationRecoveryServiceTest.java` around lines 24 - 45, Update RagIndexOperationRecoveryServiceTest to use MockitoExtension with `@Mock` fields instead of creating mocks manually in setUp, and remove the corresponding mock initializations. Preserve the recovery construction while adding lenient() only for stubs that are intentionally unused, so strict stubbing detects missing interactions such as findById.java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/branch/RagBranchOperatorAliasReconciliationService.java (1)
39-48: 🚀 Performance & Scalability | 🔵 TrivialConsider bounding the reconciliation scan.
The loop performs one pipeline call per candidate with no limit. On a large installation a single scheduled run can exceed the 5-minute fixed delay. Consider a page limit per run, or a metric for candidate count and run duration, so the backlog stays observable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/branch/RagBranchOperatorAliasReconciliationService.java` around lines 39 - 48, Bound the reconciliation work in RagBranchOperatorAliasReconciliationService by limiting the candidates processed per scheduled run, while preserving the existing publishGenerationAliases behavior for each selected candidate. Use an established page-size or batch-size configuration if available; otherwise introduce a clear bounded limit and expose candidate-count or run-duration metrics so remaining backlog stays observable.java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/branch/RagIndexOperationRecoveryService.java (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a narrower dependency than
RagOperationsService.The recovery scheduler injects the full
RagOperationsServiceonly to call the default methodgetBaseBranch. This couples a background recovery component to the RAG orchestration service and creates a future bean-cycle risk, becauseRagOperationsServiceImplalready depends onRagBranchIndexRegistryService. Extract base-branch resolution into a small stateless helper and inject that instead.Also applies to: 116-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/branch/RagIndexOperationRecoveryService.java` at line 35, Replace the RagOperationsService dependency in RagIndexOperationRecoveryService with a small stateless helper dedicated to base-branch resolution, and update the recovery scheduling logic around getBaseBranch to use that helper. Ensure the helper preserves the existing default getBaseBranch behavior, then remove the broad orchestration-service injection to avoid the dependency cycle.java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/RagBranchIndexRegistryService.java (1)
262-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove or wire
hasLiveOperation.
hasLiveOperationhas no callers, andexistsByProjectIdAndBranchNameAndStatusInis used only by this method. Remove both APIs, or usehasLiveOperationin the recovery flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/RagBranchIndexRegistryService.java` around lines 262 - 267, Remove the unused hasLiveOperation method from RagBranchIndexRegistryService and remove the corresponding existsByProjectIdAndBranchNameAndStatusIn repository API, unless the recovery flow is updated to call hasLiveOperation. Do not leave the repository query without a caller.java-ecosystem/libs/core/src/test/java/org/rostilos/codecrow/core/service/qadoc/QaDocContentParserTest.java (1)
80-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the exception contracts of
replaceShareableSections.
replaceMarkedTestCaseBodythrowsIllegalArgumentExceptionwhen the marked section is absent or when the replacement is blank.replaceEnvironmentBodythrows for a blank environment replacement. The QA handoff flow depends on these throws, but no test asserts them.Add negative tests for the three throw paths. Also cover the synthetic-heading branch, where the markers exist but contain no
Test Scenariosheading.🧪 Proposed tests
`@Test` void rejectsReplacementWhenTheMarkedSectionIsAbsent() { assertThatThrownBy(() -> QaDocContentParser.replaceShareableSections( "### 1. Change Summary\nNo markers here.\n", "https://x/#tab=a", "https://x/#tab=b")) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining("marked QA test-case section"); } `@Test` void rejectsBlankReplacements() { String markdown = """ <!-- codecrow-test-cases:start --> ### 3. Test Scenarios **Pay** (HIGH) - **Expected Result:** Ok. <!-- codecrow-test-cases:end --> """; assertThatThrownBy(() -> QaDocContentParser.replaceShareableSections(markdown, " ", "https://x/#tab=b")) .isInstanceOf(IllegalArgumentException.class); assertThatThrownBy(() -> QaDocContentParser.replaceShareableSections(markdown, "https://x/#tab=a", " ")) .isInstanceOf(IllegalArgumentException.class); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java-ecosystem/libs/core/src/test/java/org/rostilos/codecrow/core/service/qadoc/QaDocContentParserTest.java` around lines 80 - 109, Add negative tests for replaceShareableSections covering missing marked test-case sections, blank test-case replacements, and blank environment replacements, asserting IllegalArgumentException and the existing missing-section message where applicable. Also add coverage for the synthetic-heading branch by supplying markers without a Test Scenarios heading and verifying the replacement behavior.java-ecosystem/services/pipeline-agent/src/test/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocHandoffRetryPolicyTest.java (1)
13-20: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMake task-ID casing explicit in the test
QaDocHandoffRetryPolicy.shouldReusecompares task IDs case-insensitively. Use"TASK-1"in this commit/timestamp test and add a separate test named for case-insensitive task matching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java-ecosystem/services/pipeline-agent/src/test/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocHandoffRetryPolicyTest.java` around lines 13 - 20, Update reusesCurrentCommitDocumentCreatedAfterTheLastSuccessfulHandoff to pass "TASK-1" and make the case-insensitive task-ID behavior explicit; add a separate test with a name indicating case-insensitive task matching that verifies shouldReuse accepts task IDs differing only by casing.java-ecosystem/services/web-server/src/test/java/org/rostilos/codecrow/webserver/publicshare/PublicShareControllerTest.java (1)
52-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the unsupported resource-type branch.
This test only covers an unresolved token. It does not cover a resolved token whose
resourceTypehas no registered provider. Add that case before keeping the combined test name.Proposed test addition
+ when(links.resolve("unsupported")) + .thenReturn(Optional.of(new ResolvedPublicShare("unknown-preview", "internal-9"))); + + assertThat(controller.resolvePublicPreview( + new PublicShareResolveRequest("unsupported"), null) + .getStatusCode().value()).isEqualTo(404);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@java-ecosystem/services/web-server/src/test/java/org/rostilos/codecrow/webserver/publicshare/PublicShareControllerTest.java` around lines 52 - 62, Extend usesTheSameNotFoundResponseForInvalidAndUnsupportedTokens to also stub a resolved public-share token with an unregistered resourceType, then assert resolvePublicPreview returns 404 and no provider preview method is called. Keep the existing invalid-token assertions and combined test coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@java-ecosystem/libs/core/src/main/java/org/rostilos/codecrow/core/service/qadoc/QaDocContentParser.java`:
- Around line 29-33: Update ENVIRONMENT_SECTION_HEADING so the numeric heading
alternative matches only titles containing the approved environment/setup names,
rather than accepting any heading numbered 6. Preserve case-insensitive matching
and the existing unnumbered/numerically prefixed title variants, ensuring
headings such as “6. Regression Risks” are not classified as the environment
section.
In
`@java-ecosystem/libs/core/src/main/resources/db/migration/managed/V2.27.0__rag_index_active_job.sql`:
- Around line 7-8: Update the idx_rag_index_status_active_job migration to use
CREATE INDEX CONCURRENTLY, add the corresponding migration configuration with
executeInTransaction=false, and configure the Flyway run with
flyway.postgresql.transactional.lock=false.
In `@java-ecosystem/libs/public-share/pom.xml`:
- Around line 38-40: Update the managed AssertJ version in the parent
java-ecosystem/pom.xml dependency management to 3.27.7 or later, so the
test-scoped assertj-core dependency inherits the secure version. Do not change
the dependency declaration in public-share.
In
`@java-ecosystem/libs/public-share/src/test/java/org/rostilos/codecrow/publicshare/service/PublicShareLinkServiceTest.java`:
- Around line 52-57: Update
rejectsValuesThatAreNotPublicShareTokensWithoutQueryingByRawValue to call
verifyNoInteractions(repository) after both resolve assertions, and add the
corresponding static import so the test verifies the repository was not queried.
In
`@java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/branch/RagIndexOperationRecoveryService.java`:
- Around line 80-86: Update lock recovery around recoverProjections and all
lock-backed startBuild overloads so every started operation persists the exact
analysisLockKey, including the overload that currently permits null. Adjust the
lock lookup used during recovery to require the lock’s expiration timestamp to
be in the future, preventing expired rows from matching before cleanup.
In
`@java-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/branch/RagIndexOperationRecoveryServiceTest.java`:
- Around line 152-169: Update
alreadyFailedOperationPreservesStatusOwnedByANewerJob to stub jobs.findById(91L)
with the expected job result before invoking recovery. Use
operation.getJobId()’s 91L value so failDurableJob follows its intended success
path instead of triggering and swallowing a NullPointerException.
In
`@java-ecosystem/libs/task-management/src/main/java/org/rostilos/codecrow/taskmanagement/jira/cloud/JiraCloudClient.java`:
- Around line 751-770: Update trimBareUrlEnd to treat a trailing double quote as
removable punctuation alongside .,;!? so quoted bare URLs produce an href
without the closing quote. Add a regression test covering
"https://example.test/path" and verify the generated URL excludes the quote.
In
`@java-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaAutoDocListener.java`:
- Around line 602-611: Update isPublicPreviewOnlyComment to remove
COMMENT_MARKER and its PR-tracking variants along with the preview URL and label
before checking blank content. Preserve classification as true for comments
containing only an auto-document marker and preview link, and add a test
covering <!-- codecrow-qa-autodoc --> before the preview link.
In
`@java-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocPublicPreviewService.java`:
- Around line 39-51: The preview flow currently validates one document but
renders another, allowing a token to be issued before rendering fails. Update
buildTaskComment to accept and render the same QaDocDocument instance passed to
createPreviewUrl, and adjust its callers so persistedDocument is used
consistently for both operations.
In
`@java-ecosystem/services/web-server/src/main/java/org/rostilos/codecrow/webserver/publicshare/qadoc/QaDocShareProvider.java`:
- Around line 135-139: Update hasCompatibleTaskKey to return true only when both
the normalized analysis task key and documentTaskKey are non-null and equal;
return false for either missing key. Add a regression test covering a missing
document task key or analysis task key and verify the task summary is not
exposed.
In
`@python-ecosystem/inference-orchestrator/src/service/qa_documentation/qa_doc_orchestrator.py`:
- Around line 726-775: Update _ensure_test_cases to reject repair responses that
omit the codecrow test-case markers instead of wrapping the entire generated
response in a fabricated section. Require both valid start and end markers,
extract only that marked section, and raise the existing repair failure error
when either marker is missing; preserve the structured-scenario validation for
marked output.
---
Outside diff comments:
In
`@java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/RagIndexTrackingService.java`:
- Around line 38-63: Serialize project-level start transitions in
RagIndexTrackingService.markIndexingStarted before creating or updating the
status, preventing concurrent first-status creation and rejecting a live status
owned by a different activeJobId. Apply the same ownership guard in the UPDATING
transition around the sibling service site. Add tests in
RagIndexTrackingServiceTest covering conflicting active ownership and concurrent
creation of the first project status; update the listed service sites and test
range accordingly.
---
Nitpick comments:
In
`@java-ecosystem/libs/core/src/test/java/org/rostilos/codecrow/core/service/qadoc/QaDocContentParserTest.java`:
- Around line 80-109: Add negative tests for replaceShareableSections covering
missing marked test-case sections, blank test-case replacements, and blank
environment replacements, asserting IllegalArgumentException and the existing
missing-section message where applicable. Also add coverage for the
synthetic-heading branch by supplying markers without a Test Scenarios heading
and verifying the replacement behavior.
In
`@java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/branch/RagBranchOperatorAliasReconciliationService.java`:
- Around line 39-48: Bound the reconciliation work in
RagBranchOperatorAliasReconciliationService by limiting the candidates processed
per scheduled run, while preserving the existing publishGenerationAliases
behavior for each selected candidate. Use an established page-size or batch-size
configuration if available; otherwise introduce a clear bounded limit and expose
candidate-count or run-duration metrics so remaining backlog stays observable.
In
`@java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/branch/RagIndexOperationRecoveryService.java`:
- Line 35: Replace the RagOperationsService dependency in
RagIndexOperationRecoveryService with a small stateless helper dedicated to
base-branch resolution, and update the recovery scheduling logic around
getBaseBranch to use that helper. Ensure the helper preserves the existing
default getBaseBranch behavior, then remove the broad orchestration-service
injection to avoid the dependency cycle.
In
`@java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/RagBranchIndexRegistryService.java`:
- Around line 262-267: Remove the unused hasLiveOperation method from
RagBranchIndexRegistryService and remove the corresponding
existsByProjectIdAndBranchNameAndStatusIn repository API, unless the recovery
flow is updated to call hasLiveOperation. Do not leave the repository query
without a caller.
In
`@java-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/branch/RagIndexOperationRecoveryServiceTest.java`:
- Around line 24-45: Update RagIndexOperationRecoveryServiceTest to use
MockitoExtension with `@Mock` fields instead of creating mocks manually in setUp,
and remove the corresponding mock initializations. Preserve the recovery
construction while adding lenient() only for stubs that are intentionally
unused, so strict stubbing detects missing interactions such as findById.
In
`@java-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/service/RagBranchIndexRegistryServiceTest.java`:
- Around line 257-273: Extend the failIfAbandoned tests around
abandonmentClaimRechecksAHeartbeatUnderTheOperationLock with a stale operation
case that verifies true is returned and both the operation and generation
transition to FAILED, plus a SUCCEEDED-status case verifying false is returned.
Reuse the existing repository stubbing and assertions for failIfAbandoned, and
keep the current heartbeat rejection test unchanged.
In
`@java-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/service/RagOperationsServiceImplTest.java`:
- Line 805: Update the mockJob setup in RagOperationsServiceImplTest to stub
getId() with a distinct value such as 4242L, then change the expected job ID in
the assertion to that value so the test verifies ownership propagation.
In
`@java-ecosystem/services/pipeline-agent/src/test/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocHandoffRetryPolicyTest.java`:
- Around line 13-20: Update
reusesCurrentCommitDocumentCreatedAfterTheLastSuccessfulHandoff to pass "TASK-1"
and make the case-insensitive task-ID behavior explicit; add a separate test
with a name indicating case-insensitive task matching that verifies shouldReuse
accepts task IDs differing only by casing.
In
`@java-ecosystem/services/web-server/src/test/java/org/rostilos/codecrow/webserver/publicshare/PublicShareControllerTest.java`:
- Around line 52-62: Extend
usesTheSameNotFoundResponseForInvalidAndUnsupportedTokens to also stub a
resolved public-share token with an unregistered resourceType, then assert
resolvePublicPreview returns 404 and no provider preview method is called. Keep
the existing invalid-token assertions and combined test coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 596b502c-9236-4ece-ba82-6980cef9550f
📒 Files selected for processing (75)
frontendjava-ecosystem/libs/core/src/main/java/module-info.javajava-ecosystem/libs/core/src/main/java/org/rostilos/codecrow/core/model/analysis/RagIndexStatus.javajava-ecosystem/libs/core/src/main/java/org/rostilos/codecrow/core/model/rag/RagIndexOperation.javajava-ecosystem/libs/core/src/main/java/org/rostilos/codecrow/core/persistence/repository/analysis/RagIndexStatusRepository.javajava-ecosystem/libs/core/src/main/java/org/rostilos/codecrow/core/persistence/repository/rag/RagBranchIndexRepository.javajava-ecosystem/libs/core/src/main/java/org/rostilos/codecrow/core/persistence/repository/rag/RagIndexOperationRepository.javajava-ecosystem/libs/core/src/main/java/org/rostilos/codecrow/core/service/QaDocDocumentService.javajava-ecosystem/libs/core/src/main/java/org/rostilos/codecrow/core/service/qadoc/QaDocContent.javajava-ecosystem/libs/core/src/main/java/org/rostilos/codecrow/core/service/qadoc/QaDocContentParser.javajava-ecosystem/libs/core/src/main/java/org/rostilos/codecrow/core/service/qadoc/QaDocPublicShareResource.javajava-ecosystem/libs/core/src/main/java/org/rostilos/codecrow/core/service/qadoc/QaDocTestCase.javajava-ecosystem/libs/core/src/main/resources/db/migration/managed/V2.27.0__rag_index_active_job.sqljava-ecosystem/libs/core/src/main/resources/db/migration/managed/V2.28.0__rag_operation_lock_owner.sqljava-ecosystem/libs/core/src/test/java/org/rostilos/codecrow/core/service/qadoc/QaDocContentParserTest.javajava-ecosystem/libs/public-share/pom.xmljava-ecosystem/libs/public-share/src/main/java/module-info.javajava-ecosystem/libs/public-share/src/main/java/org/rostilos/codecrow/publicshare/api/IssuedPublicShare.javajava-ecosystem/libs/public-share/src/main/java/org/rostilos/codecrow/publicshare/api/ResolvedPublicShare.javajava-ecosystem/libs/public-share/src/main/java/org/rostilos/codecrow/publicshare/config/PublicShareAutoConfiguration.javajava-ecosystem/libs/public-share/src/main/java/org/rostilos/codecrow/publicshare/model/PublicShareLink.javajava-ecosystem/libs/public-share/src/main/java/org/rostilos/codecrow/publicshare/persistence/PublicShareLinkRepository.javajava-ecosystem/libs/public-share/src/main/java/org/rostilos/codecrow/publicshare/service/PublicShareLinkService.javajava-ecosystem/libs/public-share/src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.importsjava-ecosystem/libs/public-share/src/main/resources/db/migration/managed/V2.25.0__public_share_links.sqljava-ecosystem/libs/public-share/src/main/resources/db/migration/managed/V2.26.0__rename_qa_document_share_resource.sqljava-ecosystem/libs/public-share/src/test/java/org/rostilos/codecrow/publicshare/config/PublicShareAutoConfigurationContextTest.javajava-ecosystem/libs/public-share/src/test/java/org/rostilos/codecrow/publicshare/config/PublicShareAutoConfigurationTest.javajava-ecosystem/libs/public-share/src/test/java/org/rostilos/codecrow/publicshare/service/PublicShareLinkServiceTest.javajava-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/branch/BranchIndexGenerationBuildService.javajava-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/branch/BranchIndexMaintenanceService.javajava-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/branch/RagBranchOperatorAliasReconciliationService.javajava-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/branch/RagIndexOperationRecoveryService.javajava-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/RagBranchIndexRegistryService.javajava-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/RagIndexTrackingService.javajava-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/RagOperationsServiceImpl.javajava-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/VcsRagIndexingService.javajava-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/branch/BranchIndexGenerationBuildServiceTest.javajava-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/branch/RagBranchOperatorAliasReconciliationServiceTest.javajava-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/branch/RagIndexOperationRecoveryServiceTest.javajava-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/service/RagBranchIndexRegistryServiceTest.javajava-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/service/RagIndexTrackingServiceTest.javajava-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/service/RagOperationsServiceImplTest.javajava-ecosystem/libs/rag-engine/src/test/java/org/rostilos/codecrow/ragengine/service/VcsRagIndexingServiceTest.javajava-ecosystem/libs/task-management/src/main/java/org/rostilos/codecrow/taskmanagement/jira/cloud/JiraCloudClient.javajava-ecosystem/libs/task-management/src/test/java/org/rostilos/codecrow/taskmanagement/jira/cloud/JiraCloudClientTest.javajava-ecosystem/pom.xmljava-ecosystem/services/pipeline-agent/pom.xmljava-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/config/AsyncConfig.javajava-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/generic/processor/command/QaDocCommandProcessor.javajava-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaAutoDocListener.javajava-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocGenerationContext.javajava-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocGenerationService.javajava-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocHandoffRetryPolicy.javajava-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocPublicPreviewService.javajava-ecosystem/services/pipeline-agent/src/test/java/org/rostilos/codecrow/pipelineagent/config/AsyncConfigTest.javajava-ecosystem/services/pipeline-agent/src/test/java/org/rostilos/codecrow/pipelineagent/generic/processor/command/QaDocCommandProcessorTest.javajava-ecosystem/services/pipeline-agent/src/test/java/org/rostilos/codecrow/pipelineagent/qadoc/QaAutoDocListenerTest.javajava-ecosystem/services/pipeline-agent/src/test/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocHandoffRetryPolicyTest.javajava-ecosystem/services/pipeline-agent/src/test/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocPublicPreviewServiceTest.javajava-ecosystem/services/web-server/pom.xmljava-ecosystem/services/web-server/src/main/java/org/rostilos/codecrow/webserver/analysis/dto/response/QaDocDocumentResponse.javajava-ecosystem/services/web-server/src/main/java/org/rostilos/codecrow/webserver/analysis/dto/response/QaDocTestCaseResponse.javajava-ecosystem/services/web-server/src/main/java/org/rostilos/codecrow/webserver/publicshare/PublicShareController.javajava-ecosystem/services/web-server/src/main/java/org/rostilos/codecrow/webserver/publicshare/PublicSharePreviewResponse.javajava-ecosystem/services/web-server/src/main/java/org/rostilos/codecrow/webserver/publicshare/PublicShareResolveRequest.javajava-ecosystem/services/web-server/src/main/java/org/rostilos/codecrow/webserver/publicshare/PublicShareResourceProvider.javajava-ecosystem/services/web-server/src/main/java/org/rostilos/codecrow/webserver/publicshare/qadoc/QaDocPublicPreview.javajava-ecosystem/services/web-server/src/main/java/org/rostilos/codecrow/webserver/publicshare/qadoc/QaDocShareProvider.javajava-ecosystem/services/web-server/src/test/java/org/rostilos/codecrow/webserver/publicshare/PublicShareControllerTest.javajava-ecosystem/services/web-server/src/test/java/org/rostilos/codecrow/webserver/publicshare/qadoc/QaDocShareProviderTest.javapython-ecosystem/inference-orchestrator/src/api/routers/qa_documentation.pypython-ecosystem/inference-orchestrator/src/service/qa_documentation/qa_doc_orchestrator.pypython-ecosystem/inference-orchestrator/src/utils/prompts/constants_qa_doc.pypython-ecosystem/inference-orchestrator/tests/test_qa_documentation.py
| private static final Pattern ENVIRONMENT_SECTION_HEADING = Pattern.compile( | ||
| "^(?:6\\.\\s+.+|(?:\\d+\\.\\s*)?(?:Environment and Setup Notes|" | ||
| + "Setup and Environment Notes|Environment Setup Notes|Environment Notes|Setup Notes))$", | ||
| Pattern.CASE_INSENSITIVE | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict the numeric alternative in ENVIRONMENT_SECTION_HEADING.
The first alternative 6\.\s+.+ matches any heading numbered 6, regardless of its title. A document that numbers a different section as 6 (for example ## 6. Regression Risks) is then classified as the environment section. Two effects follow: parse removes that content from overviewMarkdown and returns it as environmentMarkdown, and replaceShareableSections replaces that body with the environment preview link in the Jira comment.
Require the title to name environment or setup content.
🔧 Proposed tightening
private static final Pattern ENVIRONMENT_SECTION_HEADING = Pattern.compile(
- "^(?:6\\.\\s+.+|(?:\\d+\\.\\s*)?(?:Environment and Setup Notes|"
+ "^(?:\\d+\\.\\s*)?(?:Environment and Setup Notes|"
+ "Setup and Environment Notes|Environment Setup Notes|Environment Notes|Setup Notes))$",
Pattern.CASE_INSENSITIVE
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static final Pattern ENVIRONMENT_SECTION_HEADING = Pattern.compile( | |
| "^(?:6\\.\\s+.+|(?:\\d+\\.\\s*)?(?:Environment and Setup Notes|" | |
| + "Setup and Environment Notes|Environment Setup Notes|Environment Notes|Setup Notes))$", | |
| Pattern.CASE_INSENSITIVE | |
| ); | |
| private static final Pattern ENVIRONMENT_SECTION_HEADING = Pattern.compile( | |
| "^(?:\\d+\\.\\s*)?(?:Environment and Setup Notes|" | |
| "Setup and Environment Notes|Environment Setup Notes|Environment Notes|Setup Notes))$", | |
| Pattern.CASE_INSENSITIVE | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@java-ecosystem/libs/core/src/main/java/org/rostilos/codecrow/core/service/qadoc/QaDocContentParser.java`
around lines 29 - 33, Update ENVIRONMENT_SECTION_HEADING so the numeric heading
alternative matches only titles containing the approved environment/setup names,
rather than accepting any heading numbered 6. Preserve case-insensitive matching
and the existing unnumbered/numerically prefixed title variants, ensuring
headings such as “6. Regression Risks” are not classified as the environment
section.
| CREATE INDEX IF NOT EXISTS idx_rag_index_status_active_job | ||
| ON rag_index_status(active_job_id); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'Flyway|executeInTransaction|CREATE INDEX CONCURRENTLY' java-ecosystemRepository: rostilos/CodeCrow
Length of output: 4839
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- migration files ---'
fd -i -t f 'V2.27.0__rag_index_active_job.sql|.*flyway.*|pom.xml|application.*properties|application.*yml' java-ecosystem \
| sort
printf '%s\n' '--- target migration and nearby migrations ---'
target='java-ecosystem/libs/core/src/main/resources/db/migration/managed/V2.27.0__rag_index_active_job.sql'
cat -n "$target"
printf '%s\n' '--- managed migrations containing index creation ---'
rg -n -C 3 'CREATE[[:space:]]+(UNIQUE[[:space:]]+)?INDEX|CONCURRENTLY|transaction' \
java-ecosystem/libs/core/src/main/resources/db/migration/managed
printf '%s\n' '--- Flyway configuration and dependency declarations ---'
rg -n -C 4 'flyway|Flyway|flyway-core|spring.flyway' java-ecosystem \
-g 'pom.xml' -g '*.properties' -g '*.yml' -g '*.yaml' -g '*.java' \
| head -n 400Repository: rostilos/CodeCrow
Length of output: 39005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- core application configuration ---'
cat -n java-ecosystem/libs/core/src/main/resources/application.yml
printf '%s\n' '--- root build and Spring Boot versions ---'
sed -n '1,130p' java-ecosystem/pom.xml
rg -n -C 5 'spring-boot|flyway|maven.compiler|java.version' java-ecosystem/pom.xml \
java-ecosystem/libs/core/pom.xml \
java-ecosystem/services/web-server/pom.xml \
java-ecosystem/services/pipeline-agent/pom.xml
printf '%s\n' '--- migration runner code and configuration properties ---'
rg -n -C 5 'Flyway|migrate\(|spring.flyway|flyway.locations|locations:.*migration|baseline' \
java-ecosystem -g '*.java' -g '*.yml' -g '*.yaml' -g '*.properties' -g 'pom.xml'Repository: rostilos/CodeCrow
Length of output: 38623
🌐 Web query:
Flyway 9.22.3 PostgreSQL CREATE INDEX CONCURRENTLY automatically runs outside transaction SQL migration
💡 Result:
No, Flyway does not automatically run CREATE INDEX CONCURRENTLY outside a transaction in a way that avoids potential deadlocks or hangs. While you can mark a migration to execute outside a transaction (using executeInTransaction=false), Flyway's default behavior in PostgreSQL is to acquire a transactional advisory lock to maintain state consistency, which can conflict with the non-transactional requirements of CREATE INDEX CONCURRENTLY [1][2][3]. When a migration script containing CREATE INDEX CONCURRENTLY runs, it often results in the migration process hanging or deadlocking because the transactional lock holds a connection open in a transaction state, while the concurrent index build itself requires its own independent transaction management [2][4][5]. To successfully run CREATE INDEX CONCURRENTLY in Flyway, you must typically perform the following: 1. Configure the transactional lock setting: You must set flyway.postgresql.transactional.lock to false [6][1]. This switches Flyway from using transactional advisory locks to session-level advisory locks, which allows concurrent index operations to proceed without holding an open transaction [6][1]. 2. Configuration scope: Note that flyway.postgresql.transactional.lock is a global configuration parameter [2]. Setting it to false affects all migrations in that execution run, not just the specific script containing the concurrent index [2]. While some users have reported issues where this setting does not resolve hangs in specific environments or newer versions [7][8], it remains the primary documented solution for this requirement [6][1][5].
Citations:
- 1: https://documentation.red-gate.com/flyway/reference/database-driver-reference/postgresql-database
- 2: Automatically use session-level PostgreSQL advisory lock for non-transactional migrations flyway/flyway#4267
- 3: https://documentation.red-gate.com/flyway/flyway-concepts/migrations/migration-transaction-handling
- 4: Transactional locks hanging for non-transactional migrations flyway/flyway#3497
- 5: Default Non transactional queries now running inside a transaction flyway/flyway#3500
- 6: https://documentation.red-gate.com/fd/flyway-postgresql-transactional-lock-setting-277579114.html
- 7: Flyway 10.19.0 - concurrent index creation with disabled transaction hangs flyway/flyway#3961
- 8: Transactional Lock False not working anymore for PostgreSQL flyway/flyway#3684
🌐 Web query:
site:documentation.red-gate.com/flyway/reference/database-driver-reference/postgresql-database transactional lock CREATE INDEX CONCURRENTLY Flyway 9
💡 Result:
In Flyway, attempting to run a CREATE INDEX CONCURRENTLY command within a standard transactional migration will cause issues because PostgreSQL does not permit CREATE INDEX CONCURRENTLY to be executed inside a transaction block [1]. By default, Flyway wraps migrations in a transactional lock for PostgreSQL [1]. To successfully use CREATE INDEX CONCURRENTLY, you must disable this default behavior by setting the configuration property flyway.postgresql.transactional.lock=false [1]. This change switches Flyway from using a transactional lock to a session-level lock, allowing the CREATE INDEX CONCURRENTLY statement to execute outside of a transaction [1].
Citations:
- 1: https://documentation.red-gate.com/flyway/reference/database-driver-reference/postgresql-database
🌐 Web query:
site:documentation.red-gate.com/flyway/flyway-concepts/migrations/migration-transaction-handling executeInTransaction SQL migration PostgreSQL
💡 Result:
In the context of database migration tools like Flyway, executeInTransaction is a configuration setting or method used to control whether a migration script is executed within a database transaction [1]. PostgreSQL supports transactional Data Definition Language (DDL), meaning most schema changes can be wrapped in a transaction, allowing them to be rolled back if an error occurs [1]. However, some specific PostgreSQL commands cannot be executed within a transaction block because they perform operations that are incompatible with transactional integrity, such as creating or dropping databases, tablespaces, or performing certain types of administrative maintenance [1]. Key considerations include: 1. Transactional DDL: Since PostgreSQL allows most DDL statements inside transactions, running migrations in a transaction is generally recommended to ensure atomic updates [1]. 2. Limitations: If a migration script contains statements that forbid transactional execution, the migration tool must be configured to either run the migration outside of a transaction or handle the mixed mode appropriately [1]. 3. Configuration: Tools like Flyway provide mechanisms to explicitly define this behavior. For SQL migrations, you can configure the executeInTransaction property for a specific script. For Java-based migrations, you can override the canExecuteInTransaction method within the migration class to return false if the logic requires non-transactional execution [1]. 4. Mixed Mode: If your migration suite contains a mix of transactional and non-transactional statements, you may need to enable specific settings (such as a mixed property in Flyway) to allow them to coexist in the same migration run, depending on the tool's capabilities [1].
Citations:
- 1: https://documentation.red-gate.com/flyway/flyway-concepts/migrations/migration-transaction-handling
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- available local Flyway artifacts ---'
find "${HOME}/.m2/repository/org/flywaydb" -maxdepth 5 -type f 2>/dev/null | sort || true
printf '%s\n' '--- migration location declarations ---'
rg -n -C 4 'db/migration|locations|flyway' java-ecosystem \
-g '*.yml' -g '*.yaml' -g '*.properties' -g '*.java' -g 'pom.xml'
printf '%s\n' '--- all explicit transaction-related Flyway settings ---'
rg -n -i -C 3 'execute.?in.?transaction|mixed|transactional.?lock|lock.?retry' java-ecosystem || trueRepository: rostilos/CodeCrow
Length of output: 37906
🌐 Web query:
Flyway SQL migration metadata executeInTransaction=false comment syntax
💡 Result:
Flyway does not use SQL comment syntax to set executeInTransaction=false [1][2]. Instead, you must use a separate script configuration file [3][4]. To execute a specific SQL migration without a transaction, perform the following steps: 1. Create a configuration file in the same directory as your SQL migration file [4]. 2. The configuration file must have the exact same filename as your SQL migration, but with an added.conf extension (e.g., if your migration is V1migration.sql, the config file should be V1migration.sql.conf) [5][4]. 3. Add the following line to this.conf file [6][7]: executeInTransaction=false This approach allows you to control transaction handling on a per-script basis without modifying the SQL content itself [3][4]. This is particularly useful for database systems like PostgreSQL or SQL Server where certain commands cannot execute within a transaction block [1][2].
Citations:
- 1: https://documentation.red-gate.com/flyway/flyway-concepts/migrations/migration-transaction-handling
- 2: https://github.com/flyway/flywaydb.org/blob/gh-pages/documentation/concepts/migrations.md
- 3: https://documentation.red-gate.com/flyway/reference/script-configuration
- 4: https://documentation.red-gate.com/flyway/flyway-concepts/migrations
- 5: https://stackoverflow.com/questions/58997440/how-to-disable-transactions-for-individual-migrations-in-flyway-on-sql-server
- 6: https://documentation.red-gate.com/flyway/reference/configuration/flyway-namespace/flyway-execute-in-transaction-setting
- 7: https://documentation.red-gate.com/fd/flyway-execute-in-transaction-setting-277578997.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path("java-ecosystem")
migration = root / "libs/core/src/main/resources/db/migration/managed/V2.27.0__rag_index_active_job.sql"
sql = migration.read_text()
statements = [
s.strip()
for s in re.split(r";\s*", sql)
if s.strip() and not all(not line.strip() or line.lstrip().startswith("--")
for line in s.splitlines())
]
print("migration:", migration)
print("statements:", len(statements))
for i, statement in enumerate(statements, 1):
normalized = re.sub(r"\s+", " ", statement)
print(f"{i}: {normalized}")
print("ordinary_index:", bool(re.search(
r"\bCREATE\s+INDEX\b(?!\s+CONCURRENTLY\b)", sql, re.I)))
print("concurrent_index:", bool(re.search(
r"\bCREATE\s+INDEX\s+CONCURRENTLY\b", sql, re.I)))
sidecar = migration.with_name(migration.name + ".conf")
print("sidecar_exists:", sidecar.exists(), sidecar)
settings = []
for path in root.rglob("*"):
if path.is_file() and path.suffix.lower() in {".properties", ".yml", ".yaml", ".conf", ".xml"}:
text = path.read_text(errors="replace")
if re.search(r"(?:spring\.flyway|flyway\.(?:executeInTransaction|postgresql\.transactional\.lock)|executeInTransaction)", text, re.I):
settings.append(str(path))
print("transaction_setting_files:", settings)
PYRepository: rostilos/CodeCrow
Length of output: 1042
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json
import urllib.request
api = "https://api.github.com/repos/flyway/flyway/git/trees/flyway-9.22.3?recursive=1"
request = urllib.request.Request(api, headers={"Accept": "application/vnd.github+json", "User-Agent": "readonly-review-check"})
with urllib.request.urlopen(request, timeout=20) as response:
tree = json.load(response)["tree"]
paths = [
item["path"] for item in tree
if any(term in item["path"].lower() for term in (
"postgresqldatabase", "postgresqlconfiguration", "sqlscript", "scriptconfiguration"
))
]
for path in paths:
print(path)
PYRepository: rostilos/CodeCrow
Length of output: 2441
Use a non-blocking index build for rag_index_status.
Replace CREATE INDEX with CREATE INDEX CONCURRENTLY. Add V2.27.0__rag_index_active_job.sql.conf with executeInTransaction=false, and set flyway.postgresql.transactional.lock=false for the Flyway run.
🧰 Tools
🪛 Squawk (2.61.0)
[warning] 7-8: During normal index creation, table updates are blocked, but reads are still allowed. Use concurrently to avoid blocking writes.
(require-concurrent-index-creation)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@java-ecosystem/libs/core/src/main/resources/db/migration/managed/V2.27.0__rag_index_active_job.sql`
around lines 7 - 8, Update the idx_rag_index_status_active_job migration to use
CREATE INDEX CONCURRENTLY, add the corresponding migration configuration with
executeInTransaction=false, and configure the Flyway run with
flyway.postgresql.transactional.lock=false.
Source: Linters/SAST tools
| <groupId>org.assertj</groupId> | ||
| <artifactId>assertj-core</artifactId> | ||
| <scope>test</scope> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Which AssertJ Core version first fixes GHSA-rqfh-9r24-8c9r?
💡 Result:
AssertJ Core version 3.27.7 is the first version that fixes the XXE vulnerability identified by GHSA-rqfh-9r24-8c9r (also tracked as CVE-2026-24400) [1][2][3]. This vulnerability existed in the org.assertj.core.util.xml.XmlStringPrettyFormatter class, which was used by the isXmlEqualTo(CharSequence) assertion [1][4]. Users of affected versions are advised to upgrade to 3.27.7, replace the affected assertion with XMLUnit, or avoid using the vulnerable methods with untrusted XML input [1][5].
Citations:
- 1: GHSA-rqfh-9r24-8c9r
- 2: GHSA-rqfh-9r24-8c9r
- 3: https://nvd.nist.gov/vuln/detail/CVE-2026-24400
- 4: https://osv.dev/vulnerability/CVE-2026-24400
- 5: https://guide.sonatype.com/vulnerability/CVE-2026-24400
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- matching POM files ---'
git ls-files '*pom.xml' | head -80
printf '%s\n' '--- AssertJ declarations and managed versions ---'
rg -n -C 4 'assertj-core|assertj.version|org.assertj' --glob 'pom.xml' .
printf '%s\n' '--- target POM context ---'
python3 - <<'PY'
from pathlib import Path
p = Path("java-ecosystem/libs/public-share/pom.xml")
if p.exists():
lines = p.read_text().splitlines()
for i in range(max(0, 32), min(len(lines), 46)):
print(f"{i+1:4}: {lines[i]}")
else:
print("target POM not found")
PYRepository: rostilos/CodeCrow
Length of output: 18902
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- AssertJ XML assertion usages ---'
rg -n -C 3 'isXmlEqualTo|isXml|XmlStringPrettyFormatter|assertj-core' \
--glob '*.{java,kt,groovy,xml}' java-ecosystem analysis-plugins || true
printf '%s\n' '--- POM parent and AssertJ scope facts ---'
python3 - <<'PY'
from pathlib import Path
import xml.etree.ElementTree as ET
NS = {'m': 'http://maven.apache.org/POM/4.0.0'}
paths = [
Path("java-ecosystem/pom.xml"),
Path("java-ecosystem/libs/public-share/pom.xml"),
Path("java-ecosystem/libs/test-support/pom.xml"),
]
for path in paths:
root = ET.parse(path).getroot()
parent = root.find("m:parent", NS)
print(path)
if parent is not None:
print(" parent:", parent.findtext("m:groupId", namespaces=NS),
parent.findtext("m:artifactId", namespaces=NS),
parent.findtext("m:version", namespaces=NS),
"relativePath=", parent.findtext("m:relativePath", namespaces=NS))
for dep in root.findall(".//m:dependency", NS):
if dep.findtext("m:artifactId", namespaces=NS) == "assertj-core":
print(" assertj:", {
"groupId": dep.findtext("m:groupId", namespaces=NS),
"version": dep.findtext("m:version", namespaces=NS),
"scope": dep.findtext("m:scope", namespaces=NS),
})
PY
printf '%s\n' '--- relevant source files ---'
git ls-files 'java-ecosystem/**/*.java' 'analysis-plugins/**/*.java' |
xargs -r rg -l 'isXmlEqualTo|XmlStringPrettyFormatter' || trueRepository: rostilos/CodeCrow
Length of output: 11359
🌐 Web query:
GHSA-rqfh-9r24-8c9r AssertJ Core affected versions fixed version official advisory
💡 Result:
The security advisory GHSA-rqfh-9r24-8c9r addresses an XML External Entity (XXE) vulnerability in AssertJ Core, identified as CVE-2026-24400 [1][2]. Affected Versions: The vulnerability affects AssertJ Core versions from 1.4.0 up to and including 3.27.6 [1][3][4]. Fixed Version: The issue is resolved in version 3.27.7 [1][5]. Advisory Details: The vulnerability exists in org.assertj.core.util.xml.XmlStringPrettyFormatter because the toXmlDocument(String) method initializes DocumentBuilderFactory with default settings, failing to disable DTDs or external entities [1][2][6]. This formatter is utilized by the isXmlEqualTo(CharSequence) assertion [1][6]. The advisory notes that applications are only vulnerable if they use untrusted XML input with either the isXmlEqualTo(CharSequence) method (from org.assertj.core.api.AbstractCharSequenceAssert) or the xmlPrettyFormat(String) method (from org.assertj.core.util.xml.XmlStringPrettyFormatter) [1][2][4]. Recommendations: Users of affected versions are advised to take one of the following actions, listed in order of preference: 1. Replace isXmlEqualTo(CharSequence) with XMLUnit [1][2]. 2. Upgrade to version 3.27.7 [1][6]. 3. Avoid using isXmlEqualTo(CharSequence) or XmlStringPrettyFormatter with untrusted input [1][4]. Additionally, the isXmlEqualTo(CharSequence) assertion was deprecated in version 3.18.0 in favor of XMLUnit and is scheduled for removal in version 4.0 [1][2][6]. In version 3.27.7, XmlStringPrettyFormatter has also been deprecated [1][2].
Citations:
- 1: GHSA-rqfh-9r24-8c9r
- 2: GHSA-rqfh-9r24-8c9r
- 3: https://osv.dev/vulnerability/GHSA-rqfh-9r24-8c9r
- 4: https://nvd.nist.gov/vuln/detail/cve-2026-24400
- 5: https://github.com/assertj/assertj/releases
- 6: https://nvd.nist.gov/vuln/detail/CVE-2026-24400
Update the managed AssertJ version in java-ecosystem/pom.xml.
AssertJ 3.25.3 is affected by GHSA-rqfh-9r24-8c9r. Upgrade to 3.27.7 or later. Exploitation requires untrusted XML passed to the affected XML assertion methods.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@java-ecosystem/libs/public-share/pom.xml` around lines 38 - 40, Update the
managed AssertJ version in the parent java-ecosystem/pom.xml dependency
management to 3.27.7 or later, so the test-scoped assertj-core dependency
inherits the secure version. Do not change the dependency declaration in
public-share.
Source: Linters/SAST tools
| void rejectsValuesThatAreNotPublicShareTokensWithoutQueryingByRawValue() { | ||
| PublicShareLinkService service = new PublicShareLinkService(repository, secureRandom); | ||
|
|
||
| assertThat(service.resolve("eyJhbGciOiJIUzI1NiJ9.jwt.payload")).isEmpty(); | ||
| assertThat(service.resolve("ccs_short")).isEmpty(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
test_file=$(fd -t f 'PublicShareLinkServiceTest\.java$' . | head -n 1)
service_file=$(fd -t f 'PublicShareLinkService\.java$' . | head -n 1)
printf '%s\n' "TEST_FILE=$test_file"
printf '%s\n' "SERVICE_FILE=$service_file"
printf '%s\n' '--- test outline ---'
ast-grep outline "$test_file" || true
printf '%s\n' '--- service outline ---'
ast-grep outline "$service_file" || true
printf '%s\n' '--- test setup and target test ---'
rg -n -C 18 'repository|secureRandom|rejectsValuesThatAreNotPublicShareTokensWithoutQueryingByRawValue|findByTokenHash|verifyNoInteractions|verify\(' "$test_file"
printf '%s\n' '--- service token resolution methods ---'
rg -n -C 25 'resolve|findByTokenHash|repository' "$service_file"Repository: rostilos/CodeCrow
Length of output: 7985
Add verifyNoInteractions(repository) after both assertions.
The assertions alone do not prove that resolve skipped the repository. Add the corresponding static import.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@java-ecosystem/libs/public-share/src/test/java/org/rostilos/codecrow/publicshare/service/PublicShareLinkServiceTest.java`
around lines 52 - 57, Update
rejectsValuesThatAreNotPublicShareTokensWithoutQueryingByRawValue to call
verifyNoInteractions(repository) after both resolve assertions, and add the
corresponding static import so the test verifies the repository was not queried.
| for (var operation : registryService.findFailedOperationsWithActiveProjections()) { | ||
| String diagnostic = operation.getErrorMessage() != null | ||
| && !operation.getErrorMessage().isBlank() | ||
| ? operation.getErrorMessage() | ||
| : "RAG generation failed before its durable projections were terminalized"; | ||
| recoverProjections(operation, diagnostic); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the query behind findFailedOperationsWithActiveProjections.
fd -t f 'RagIndexOperationRepository.java' --exec cat -n {}Repository: rostilos/CodeCrow
Length of output: 3395
🏁 Script executed:
#!/bin/bash
set -eu
service=$(fd -t f 'RagIndexOperationRecoveryService.java' | head -n 1)
printf '%s\n' "=== $service ==="
cat -n "$service"
printf '%s\n' '=== related recovery symbols ==='
rg -n -S \
'recoverProjections|terminalizePrimaryStatus|findFailedOperationsWithActiveProjections|class RagIndexOperationRecoveryService|activeJobId|analysis_lock|RAG_INDEXING' \
java-ecosystem/libs java-ecosystem 2>/dev/null | head -n 240Repository: rostilos/CodeCrow
Length of output: 28307
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '=== recovery tests ==='
test_file=$(fd -t f 'RagIndexOperationRecoveryServiceTest.java' | head -n 1)
cat -n "$test_file"
printf '%s\n' '=== job failure implementation ==='
fd -t f 'JobService.java' --exec rg -n -A 45 -B 12 \
'failJob|isTerminal|class JobService' {}
printf '%s\n' '=== lock release implementation ==='
fd -t f 'AnalysisLockService.java' --exec rg -n -A 60 -B 18 \
'releaseLock|acquireLock|lockKey|commitHash|expiresAt' {}
printf '%s\n' '=== tracking terminalization implementation ==='
fd -t f 'RagIndexTrackingService.java' --exec rg -n -A 45 -B 12 \
'markIndexingFailed|markIncrementalUpdateFailed|getIndexStatus|activeJobId' {}Repository: rostilos/CodeCrow
Length of output: 48061
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '=== analysis lock key assignments ==='
rg -n -A 12 -B 12 \
'setAnalysisLockKey|getAnalysisLockKey|analysisLockKey|new RagIndexOperation|RagIndexOperation\(' \
java-ecosystem/libs --glob '*.java' --glob '*.sql' | head -n 260
printf '%s\n' '=== operation model and schema ==='
fd -t f 'RagIndexOperation.java' --exec cat -n {}
rg -n -A 10 -B 8 \
'CREATE TABLE.*rag_index_operation|analysis_lock_key|to_revision|job_id' \
java-ecosystem/libs/core/src/main/resources --glob '*.sql' | head -n 220
printf '%s\n' '=== lock query and cleanup predicates ==='
fd -t f 'AnalysisLockRepository.java' --exec rg -n -A 35 -B 12 \
'deleteByLockKey|existsActiveLock|findByLockKey|commitHash|expiresAt' {}Repository: rostilos/CodeCrow
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '=== registry lock-key lifecycle ==='
file=$(fd -t f 'RagBranchIndexRegistryService.java' | head -n 1)
rg -n -A 18 -B 18 \
'registerBuild|startBuild|setAnalysisLockKey|analysisLockKey' "$file"
printf '%s\n' '=== schema definition for analysis_lock ==='
rg -n -A 16 -B 6 \
'CREATE TABLE.*analysis_lock|expires_at|lock_key|commit_hash' \
java-ecosystem/libs/core/src/main/resources/db/migration --glob '*.sql' | head -n 140
printf '%s\n' '=== operation schema migration ==='
rg -n -A 20 -B 8 \
'rag_index_operation|analysis_lock_key' \
java-ecosystem/libs/core/src/main/resources/db/migration --glob '*.sql' | head -n 160Repository: rostilos/CodeCrow
Length of output: 35488
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -A 4 -B 4 \
'\.startBuild\(' \
java-ecosystem/libs --glob '*.java' | head -n 180Repository: rostilos/CodeCrow
Length of output: 8342
Make lock recovery converge when the lock owner key is missing.
The job and primary-status paths converge. A startBuild overload can store a null analysisLockKey, so recovery cannot release an existing lock. The lock query also matches expired rows until cleanup runs. Pass the exact key for every lock-backed start and require an unexpired lock in the query.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/branch/RagIndexOperationRecoveryService.java`
around lines 80 - 86, Update lock recovery around recoverProjections and all
lock-backed startBuild overloads so every started operation persists the exact
analysisLockKey, including the overload that currently permits null. Adjust the
lock lookup used during recovery to require the lock’s expiration timestamp to
be in the future, preventing expired rows from matching before cleanup.
| private static int trimBareUrlEnd(String text, int start, int end) { | ||
| int trimmedEnd = end; | ||
| while (trimmedEnd > start) { | ||
| char trailing = text.charAt(trimmedEnd - 1); | ||
| if (".,;!?".indexOf(trailing) >= 0) { | ||
| trimmedEnd--; | ||
| continue; | ||
| } | ||
| char opening = switch (trailing) { | ||
| case ')' -> '('; | ||
| case ']' -> '['; | ||
| case '}' -> '{'; | ||
| default -> '\0'; | ||
| }; | ||
| if (opening == '\0' || !hasUnmatchedClosingDelimiter( | ||
| text, start, trimmedEnd, opening, trailing)) { | ||
| break; | ||
| } | ||
| trimmedEnd--; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim trailing double quotes from bare URLs.
If a URL is enclosed in double quotes, trimBareUrlEnd keeps the closing ". The generated href then differs from the URL and includes punctuation.
Trim " with the other trailing punctuation. Add a regression test for "https://example.test/path".
Proposed fix
- if (".,;!?".indexOf(trailing) >= 0) {
+ if (".,;!?\"".indexOf(trailing) >= 0) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static int trimBareUrlEnd(String text, int start, int end) { | |
| int trimmedEnd = end; | |
| while (trimmedEnd > start) { | |
| char trailing = text.charAt(trimmedEnd - 1); | |
| if (".,;!?".indexOf(trailing) >= 0) { | |
| trimmedEnd--; | |
| continue; | |
| } | |
| char opening = switch (trailing) { | |
| case ')' -> '('; | |
| case ']' -> '['; | |
| case '}' -> '{'; | |
| default -> '\0'; | |
| }; | |
| if (opening == '\0' || !hasUnmatchedClosingDelimiter( | |
| text, start, trimmedEnd, opening, trailing)) { | |
| break; | |
| } | |
| trimmedEnd--; | |
| } | |
| private static int trimBareUrlEnd(String text, int start, int end) { | |
| int trimmedEnd = end; | |
| while (trimmedEnd > start) { | |
| char trailing = text.charAt(trimmedEnd - 1); | |
| if (".,;!?\"".indexOf(trailing) >= 0) { | |
| trimmedEnd--; | |
| continue; | |
| } | |
| char opening = switch (trailing) { | |
| case ')' -> '('; | |
| case ']' -> '['; | |
| case '}' -> '{'; | |
| default -> '\0'; | |
| }; | |
| if (opening == '\0' || !hasUnmatchedClosingDelimiter( | |
| text, start, trimmedEnd, opening, trailing)) { | |
| break; | |
| } | |
| trimmedEnd--; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@java-ecosystem/libs/task-management/src/main/java/org/rostilos/codecrow/taskmanagement/jira/cloud/JiraCloudClient.java`
around lines 751 - 770, Update trimBareUrlEnd to treat a trailing double quote
as removable punctuation alongside .,;!? so quoted bare URLs produce an href
without the closing quote. Add a regression test covering
"https://example.test/path" and verify the generated URL excludes the quote.
| public static boolean isPublicPreviewOnlyComment(String body) { | ||
| if (body == null || (!body.contains("/share#token=ccs_") | ||
| && !body.contains("/share?token=ccs_"))) { | ||
| return false; | ||
| } | ||
| String withoutPreviewLink = body | ||
| .replaceAll("https?://\\S+/share(?:#|\\?)token=ccs_[A-Za-z0-9_-]+", "") | ||
| .replace("View QA test cases in CodeCrow", "") | ||
| .replaceAll("[\\[\\]()\\s]", ""); | ||
| return withoutPreviewLink.isBlank(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove the auto-document marker before classifying a link-only comment.
findCommentByMarker(..., COMMENT_MARKER_PREFIX) selects comments that contain the CodeCrow marker. Lines 607-611 remove the preview URL and label but retain that marker. The method then returns false for a marker plus preview-link-only comment. This causes the fallback to use a public link as previous QA documentation.
Strip COMMENT_MARKER and its PR-tracking variants before the blank-content check. Add a test with <!-- codecrow-qa-autodoc --> before the preview link.
Proposed fix
String withoutPreviewLink = body
.replaceAll("https?://\\S+/share(?:#|\\?)token=ccs_[A-Za-z0-9_-]+", "")
.replace("View QA test cases in CodeCrow", "")
+ .replaceAll("<!--\\s*codecrow-qa-autodoc(?::[^>]*)?\\s*-->", "")
.replaceAll("[\\[\\]()\\s]", "");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static boolean isPublicPreviewOnlyComment(String body) { | |
| if (body == null || (!body.contains("/share#token=ccs_") | |
| && !body.contains("/share?token=ccs_"))) { | |
| return false; | |
| } | |
| String withoutPreviewLink = body | |
| .replaceAll("https?://\\S+/share(?:#|\\?)token=ccs_[A-Za-z0-9_-]+", "") | |
| .replace("View QA test cases in CodeCrow", "") | |
| .replaceAll("[\\[\\]()\\s]", ""); | |
| return withoutPreviewLink.isBlank(); | |
| public static boolean isPublicPreviewOnlyComment(String body) { | |
| if (body == null || (!body.contains("/share#token=ccs_") | |
| && !body.contains("/share?token=ccs_"))) { | |
| return false; | |
| } | |
| String withoutPreviewLink = body | |
| .replaceAll("https?://\\S+/share(?:#|\\?)token=ccs_[A-Za-z0-9_-]+", "") | |
| .replace("View QA test cases in CodeCrow", "") | |
| .replaceAll("<!--\\s*codecrow-qa-autodoc(?::[^>]*)?\\s*-->", "") | |
| .replaceAll("[\\[\\]()\\s]", ""); | |
| return withoutPreviewLink.isBlank(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@java-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaAutoDocListener.java`
around lines 602 - 611, Update isPublicPreviewOnlyComment to remove
COMMENT_MARKER and its PR-tracking variants along with the preview URL and label
before checking blank content. Preserve classification as true for comments
containing only an auto-document marker and preview link, and add a test
covering <!-- codecrow-qa-autodoc --> before the preview link.
| public String buildTaskComment(String qaDocument, String previewUrl) { | ||
| if (previewUrl == null || previewUrl.isBlank()) { | ||
| throw new IllegalArgumentException("A public preview URL is required."); | ||
| } | ||
| String normalizedPreviewUrl = previewUrl.trim(); | ||
| return QaAutoDocListener.COMMENT_MARKER | ||
| + "\n\n" | ||
| + QaDocContentParser.replaceShareableSections( | ||
| qaDocument, | ||
| withTab(normalizedPreviewUrl, "test-cases"), | ||
| withTab(normalizedPreviewUrl, "environment") | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which markdown the callers pass to createPreviewUrl and buildTaskComment.
rg -n -C 8 'createPreviewUrl|buildTaskComment' --type=java -g '!**/test/**'Repository: rostilos/CodeCrow
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -eu
file='java-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocPublicPreviewService.java'
printf '%s\n' '--- target file ---'
sed -n '1,140p' "$file"
printf '%s\n' '--- related Java files ---'
rg -l --type=java 'QaDocPublicPreviewService|createPreviewUrl|buildTaskComment' . || true
printf '%s\n' '--- method references ---'
rg -n -C 12 --type=java 'createPreviewUrl|buildTaskComment|QaDocPublicPreviewService' . || trueRepository: rostilos/CodeCrow
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- production call sites ---'
rg -n -C 18 --type=java '\.buildTaskComment\(|\.createPreviewUrl\(' \
java-ecosystem/services/pipeline-agent/src/main/java
printf '%s\n' '--- parser declarations and implementation ---'
rg -n -C 20 --type=java 'replaceShareableSections|parseMarkedTestCases' \
java-ecosystem
printf '%s\n' '--- relevant listener and processor sections ---'
sed -n '180,330p' java-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaAutoDocListener.java
sed -n '1,260p' java-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/generic/processor/command/QaDocCommandProcessor.javaRepository: rostilos/CodeCrow
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
parser=$(rg -l --type=java 'class QaDocContentParser' java-ecosystem)
printf '%s\n' "--- parser: $parser ---"
rg -n -A 80 -B 10 'replaceShareableSections|parseMarkedTestCases' "$parser"
printf '%s\n' '--- listener persistence helper ---'
rg -n -A 90 -B 15 'upsertQaDocDocument|pendingHandoffDocument|qaDocument' \
java-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaAutoDocListener.java
printf '%s\n' '--- command processor persistence helper ---'
rg -n -A 90 -B 15 'upsertQaDocDocument|pendingHandoffDocument|qaDocument' \
java-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/generic/processor/command/QaDocCommandProcessor.javaRepository: rostilos/CodeCrow
Length of output: 878
🏁 Script executed:
#!/bin/bash
set -eu
parser='java-ecosystem/libs/core/src/main/java/org/rostilos/codecrow/core/service/qadoc/QaDocContentParser.java'
printf '%s\n' "--- parser: $parser ---"
rg -n -A 90 -B 10 'replaceShareableSections|parseMarkedTestCases' "$parser"
printf '%s\n' '--- listener persistence references ---'
rg -n -A 55 -B 12 'upsertQaDocDocument|pendingHandoffDocument' \
java-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaAutoDocListener.java
printf '%s\n' '--- command processor persistence references ---'
rg -n -A 55 -B 12 'upsertQaDocDocument|pendingHandoffDocument' \
java-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/generic/processor/command/QaDocCommandProcessor.javaRepository: rostilos/CodeCrow
Length of output: 32385
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
service = Path(
"java-ecosystem/services/pipeline-agent/src/main/java/"
"org/rostilos/codecrow/pipelineagent/qadoc/QaDocPublicPreviewService.java"
).read_text()
listener = Path(
"java-ecosystem/services/pipeline-agent/src/main/java/"
"org/rostilos/codecrow/pipelineagent/qadoc/QaAutoDocListener.java"
).read_text()
processor = Path(
"java-ecosystem/services/pipeline-agent/src/main/java/"
"org/rostilos/codecrow/pipelineagent/generic/processor/command/"
"QaDocCommandProcessor.java"
).read_text()
parser = Path(
"java-ecosystem/libs/core/src/main/java/"
"org/rostilos/codecrow/core/service/qadoc/QaDocContentParser.java"
).read_text()
assert service.index("public String createPreviewUrl") < service.index("public String buildTaskComment")
assert service.index("publicShares.issue") < service.index("buildTaskComment")
assert "createPreviewUrl(persistedDocument.get())" in listener
assert "buildTaskComment(qaDocument, previewUrl)" in listener
assert "createPreviewUrl(persistedDocument.get())" in processor
assert "buildTaskComment(qaDocument, previewUrl)" in processor
assert 'if (section == null)' in parser
assert 'throw new IllegalArgumentException("A marked QA test-case section is required.")' in parser
print("production callers use persistedDocument for URL validation and qaDocument for comment rendering")
print("token issuance precedes comment rendering")
print("comment rendering rejects input without a marked test-case section")
PYRepository: rostilos/CodeCrow
Length of output: 261
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
service = Path(
"java-ecosystem/services/pipeline-agent/src/main/java/"
"org/rostilos/codecrow/pipelineagent/qadoc/QaDocPublicPreviewService.java"
).read_text()
listener = Path(
"java-ecosystem/services/pipeline-agent/src/main/java/"
"org/rostilos/codecrow/pipelineagent/qadoc/QaAutoDocListener.java"
).read_text()
processor = Path(
"java-ecosystem/services/pipeline-agent/src/main/java/"
"org/rostilos/codecrow/pipelineagent/generic/processor/command/"
"QaDocCommandProcessor.java"
).read_text()
parser = Path(
"java-ecosystem/libs/core/src/main/java/"
"org/rostilos/codecrow/core/service/qadoc/QaDocContentParser.java"
).read_text()
assert service.index("public String createPreviewUrl") < service.index("public String buildTaskComment")
assert service.index("publicShareLinkService.issue") < service.index("buildTaskComment")
assert "createPreviewUrl(persistedDocument.get())" in listener
assert "buildTaskComment(qaDocument, previewUrl)" in listener
assert "createPreviewUrl(persistedDocument.get())" in processor
assert "buildTaskComment(qaDocument, previewUrl)" in processor
assert 'if (section == null)' in parser
assert 'throw new IllegalArgumentException("A marked QA test-case section is required.")' in parser
print("production callers use persistedDocument for URL validation and qaDocument for comment rendering")
print("token issuance precedes comment rendering")
print("comment rendering rejects input without a marked test-case section")
PYRepository: rostilos/CodeCrow
Length of output: 361
Use the same QaDocDocument for preview validation and comment rendering.
The callers pass persistedDocument to createPreviewUrl but pass qaDocument to buildTaskComment. If qaDocument lacks the marked test-case section, rendering throws after PublicShareLinkService.issue creates the token. Accept QaDocDocument in buildTaskComment, or combine both operations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@java-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocPublicPreviewService.java`
around lines 39 - 51, The preview flow currently validates one document but
renders another, allowing a token to be issued before rendering fails. Update
buildTaskComment to accept and render the same QaDocDocument instance passed to
createPreviewUrl, and adjust its callers so persistedDocument is used
consistently for both operations.
| private boolean hasCompatibleTaskKey(CodeAnalysis analysis, String documentTaskKey) { | ||
| String analysisTaskKey = normalize(analysis.getTaskId()); | ||
| return documentTaskKey == null || analysisTaskKey == null | ||
| || Objects.equals(documentTaskKey, analysisTaskKey); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require matching task keys before exposing the task summary.
When either task key is null, this method returns true. findTaskSummary then includes the analysis summary in the public preview. A missing task key cannot prove that the analysis belongs to the same task.
Return false unless both normalized keys are present and equal. Add a regression test for a missing document or analysis task key.
Proposed fix
private boolean hasCompatibleTaskKey(CodeAnalysis analysis, String documentTaskKey) {
String analysisTaskKey = normalize(analysis.getTaskId());
- return documentTaskKey == null || analysisTaskKey == null
- || Objects.equals(documentTaskKey, analysisTaskKey);
+ return documentTaskKey != null
+ && Objects.equals(documentTaskKey, analysisTaskKey);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private boolean hasCompatibleTaskKey(CodeAnalysis analysis, String documentTaskKey) { | |
| String analysisTaskKey = normalize(analysis.getTaskId()); | |
| return documentTaskKey == null || analysisTaskKey == null | |
| || Objects.equals(documentTaskKey, analysisTaskKey); | |
| } | |
| private boolean hasCompatibleTaskKey(CodeAnalysis analysis, String documentTaskKey) { | |
| String analysisTaskKey = normalize(analysis.getTaskId()); | |
| return documentTaskKey != null | |
| && Objects.equals(documentTaskKey, analysisTaskKey); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@java-ecosystem/services/web-server/src/main/java/org/rostilos/codecrow/webserver/publicshare/qadoc/QaDocShareProvider.java`
around lines 135 - 139, Update hasCompatibleTaskKey to return true only when
both the normalized analysis task key and documentTaskKey are non-null and
equal; return false for either missing key. Add a regression test covering a
missing document task key or analysis task key and verify the task summary is
not exposed.
| async def _ensure_test_cases( | ||
| self, | ||
| documentation: str, | ||
| placeholders: Dict[str, str], | ||
| ) -> str: | ||
| """Guarantee an independently extractable test-case section. | ||
|
|
||
| Normal generation is instructed to emit stable invisible markers. If a | ||
| custom template or model response omits them, one focused repair call | ||
| generates only the missing section without narrowing the requested test | ||
| coverage. | ||
| """ | ||
| if self._contains_extractable_test_cases(documentation): | ||
| return self._normalize_test_case_markers(documentation) | ||
|
|
||
| repair_placeholders = dict(placeholders) | ||
| raw_diff = repair_placeholders.get("diff", "") | ||
| max_repair_diff = 120_000 | ||
| if len(raw_diff) > max_repair_diff: | ||
| repair_placeholders["diff"] = ( | ||
| raw_diff[:max_repair_diff] | ||
| + f"\n\n... (diff truncated — {len(raw_diff)} chars total, " | ||
| f"showing first {max_repair_diff})" | ||
| ) | ||
|
|
||
| logger.warning("QA doc omitted extractable test cases; running focused repair generation") | ||
| prompt = QA_DOC_TEST_CASES_REPAIR_PROMPT.format(**repair_placeholders) | ||
| response = await self.llm.ainvoke([ | ||
| {"role": "system", "content": QA_DOC_SYSTEM_PROMPT}, | ||
| {"role": "user", "content": prompt}, | ||
| ]) | ||
| generated = self._extract_text(response).strip() | ||
| if not generated: | ||
| raise ValueError("Test-case generation returned no content") | ||
|
|
||
| start_marker = "<!-- codecrow-test-cases:start -->" | ||
| end_marker = "<!-- codecrow-test-cases:end -->" | ||
| start = generated.find(start_marker) | ||
| end = generated.find(end_marker, start + len(start_marker)) if start >= 0 else -1 | ||
| if start >= 0 and end >= 0: | ||
| test_case_section = generated[start:end + len(end_marker)] | ||
| else: | ||
| test_case_section = f"{start_marker}\n{generated}\n{end_marker}" | ||
|
|
||
| if not self._contains_extractable_test_cases(test_case_section): | ||
| raise ValueError("Test-case generation returned no structured scenarios") | ||
|
|
||
| return self._normalize_test_case_markers( | ||
| documentation.rstrip() + "\n\n" + test_case_section.strip() | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject unmarked repair output.
If repair generation omits the markers, Lines 767-768 wrap the complete model response in the public test-case boundary. The response can contain preambles or non-test-case technical content. Require one valid marked section, or fail the repair.
Proposed fix
if start >= 0 and end >= 0:
test_case_section = generated[start:end + len(end_marker)]
else:
- test_case_section = f"{start_marker}\n{generated}\n{end_marker}"
+ raise ValueError("Test-case generation omitted required markers")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def _ensure_test_cases( | |
| self, | |
| documentation: str, | |
| placeholders: Dict[str, str], | |
| ) -> str: | |
| """Guarantee an independently extractable test-case section. | |
| Normal generation is instructed to emit stable invisible markers. If a | |
| custom template or model response omits them, one focused repair call | |
| generates only the missing section without narrowing the requested test | |
| coverage. | |
| """ | |
| if self._contains_extractable_test_cases(documentation): | |
| return self._normalize_test_case_markers(documentation) | |
| repair_placeholders = dict(placeholders) | |
| raw_diff = repair_placeholders.get("diff", "") | |
| max_repair_diff = 120_000 | |
| if len(raw_diff) > max_repair_diff: | |
| repair_placeholders["diff"] = ( | |
| raw_diff[:max_repair_diff] | |
| + f"\n\n... (diff truncated — {len(raw_diff)} chars total, " | |
| f"showing first {max_repair_diff})" | |
| ) | |
| logger.warning("QA doc omitted extractable test cases; running focused repair generation") | |
| prompt = QA_DOC_TEST_CASES_REPAIR_PROMPT.format(**repair_placeholders) | |
| response = await self.llm.ainvoke([ | |
| {"role": "system", "content": QA_DOC_SYSTEM_PROMPT}, | |
| {"role": "user", "content": prompt}, | |
| ]) | |
| generated = self._extract_text(response).strip() | |
| if not generated: | |
| raise ValueError("Test-case generation returned no content") | |
| start_marker = "<!-- codecrow-test-cases:start -->" | |
| end_marker = "<!-- codecrow-test-cases:end -->" | |
| start = generated.find(start_marker) | |
| end = generated.find(end_marker, start + len(start_marker)) if start >= 0 else -1 | |
| if start >= 0 and end >= 0: | |
| test_case_section = generated[start:end + len(end_marker)] | |
| else: | |
| test_case_section = f"{start_marker}\n{generated}\n{end_marker}" | |
| if not self._contains_extractable_test_cases(test_case_section): | |
| raise ValueError("Test-case generation returned no structured scenarios") | |
| return self._normalize_test_case_markers( | |
| documentation.rstrip() + "\n\n" + test_case_section.strip() | |
| ) | |
| async def _ensure_test_cases( | |
| self, | |
| documentation: str, | |
| placeholders: Dict[str, str], | |
| ) -> str: | |
| """Guarantee an independently extractable test-case section. | |
| Normal generation is instructed to emit stable invisible markers. If a | |
| custom template or model response omits them, one focused repair call | |
| generates only the missing section without narrowing the requested test | |
| coverage. | |
| """ | |
| if self._contains_extractable_test_cases(documentation): | |
| return self._normalize_test_case_markers(documentation) | |
| repair_placeholders = dict(placeholders) | |
| raw_diff = repair_placeholders.get("diff", "") | |
| max_repair_diff = 120_000 | |
| if len(raw_diff) > max_repair_diff: | |
| repair_placeholders["diff"] = ( | |
| raw_diff[:max_repair_diff] | |
| f"\n\n... (diff truncated — {len(raw_diff)} chars total, " | |
| f"showing first {max_repair_diff})" | |
| ) | |
| logger.warning("QA doc omitted extractable test cases; running focused repair generation") | |
| prompt = QA_DOC_TEST_CASES_REPAIR_PROMPT.format(**repair_placeholders) | |
| response = await self.llm.ainvoke([ | |
| {"role": "system", "content": QA_DOC_SYSTEM_PROMPT}, | |
| {"role": "user", "content": prompt}, | |
| ]) | |
| generated = self._extract_text(response).strip() | |
| if not generated: | |
| raise ValueError("Test-case generation returned no content") | |
| start_marker = "<!-- codecrow-test-cases:start -->" | |
| end_marker = "<!-- codecrow-test-cases:end -->" | |
| start = generated.find(start_marker) | |
| end = generated.find(end_marker, start + len(start_marker)) if start >= 0 else -1 | |
| if start >= 0 and end >= 0: | |
| test_case_section = generated[start:end + len(end_marker)] | |
| else: | |
| raise ValueError("Test-case generation omitted required markers") | |
| if not self._contains_extractable_test_cases(test_case_section): | |
| raise ValueError("Test-case generation returned no structured scenarios") | |
| return self._normalize_test_case_markers( | |
| documentation.rstrip() + "\n\n" + test_case_section.strip() | |
| ) |
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 762-762: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: generated.find(start_marker)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 763-763: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: generated.find(end_marker, start + len(start_marker))
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@python-ecosystem/inference-orchestrator/src/service/qa_documentation/qa_doc_orchestrator.py`
around lines 726 - 775, Update _ensure_test_cases to reject repair responses
that omit the codecrow test-case markers instead of wrapping the entire
generated response in a fabricated section. Require both valid start and end
markers, extract only that marked section, and raise the existing repair failure
error when either marker is missing; preserve the structured-scenario validation
for marked output.
- renew and fence PR, branch, exact-generation, and legacy RAG leases - recover abandoned jobs without corrupting newer owners or checkpoints - fix detached RAG generation proxies by using scalar projections - make exact generation admission, publication, and status recovery durable - preserve legacy Qdrant collections and repair payload indexes in place - harden PR overlay cleanup, alias reconciliation, and transient cleanup - handle excluded incremental files without failing the indexing job - treat lock contention as a skipped update instead of a failure - drain admitted queue and streaming work during graceful shutdown - serialize terminal events and add bounded Redis event retention - degrade optional RAG context without failing the core review - remove unused QA documentation RAG overlays and credential forwarding - suppress duplicate diagnostics while retaining owner-level failures - add recovery configuration samples, migration, and regression coverage
fix(reliability): stabilize analysis queues and RAG lifecycle
- prevent PR reviews from synchronously starting or waiting for repository RAG builds - clarify and rate-limit cross-type PR/branch dependency wait events - keep unrelated PRs and branches independently executable - harden analysis locks, leases, ownership fencing, and durable job recovery - fix detached RAG generation proxy failures during incremental updates - build exact generations from complete revision-pinned repository snapshots - reuse compatible vectors from the previous immutable generation - make RAG cleanup, alias reconciliation, and queue shutdown reliable - bound repeated outage diagnostics and remove duplicate terminal events - preserve compatibility with existing Qdrant collections and configuration - add regression coverage across Pipeline Agent, RAG, and Inference services
fix: stabilize analysis orchestration and RAG generation lifecycle
|
| Status | PASS WITH WARNINGS |
| Risk Level | MEDIUM |
| Review Coverage | 17 files analyzed in depth |
| Confidence | MEDIUM |
Executive Summary
This release-candidate PR updates RAG index generation and collection targeting, analysis gating and wait handling, job/index lifecycle behavior, and QA document processing. The reviewed changes show no critical or high-severity blockers, but several medium-risk correctness, compatibility, error-handling, and test-alignment concerns remain across these areas. No task context was provided, so task-coverage confidence is based solely on the reviewed PR changes.
Recommendation
Decision: PASS WITH WARNINGS
The PR may proceed if the identified warnings are addressed or explicitly accepted, with particular attention to backward compatibility for existing index state, recovery behavior, parser correctness, and failing or mismatched test expectations. Further validation of affected integration paths is recommended before release.
Issues Overview
| Severity | Count | |
|---|---|---|
| 🟡 Medium | 6 | Issues that should be addressed |
| 🔵 Low | 2 | Minor issues and improvements |
| ✅ Resolved | 1 | Resolved issues |
Analysis completed on 2026-08-12 21:13:40 | View Full Report | Pull Request
📋 Detailed Issues (8)
🟡 Medium Severity Issues
Id on Platform: 4054
Category: 🧪 Testing
File: .../branch/BranchAnalysisGateServiceTest.java:115
Assertions expect fields absent from wait events
The updated matcher requires waitingJobType and blockingJobType to be present in the emitted event. However, the current BranchAnalysisGateService.emitWait and emitBranchWait implementations populate the event with type, state, message, branch name, waited time, and optionally PR number, but do not add either of these fields. Consequently, mergeWaitsOnlyForNewestAttemptOfItsOwnPr and the corresponding branch-wait test will fail whenever a wait event is emitted.
💡 Suggested Fix
Either remove the two unsupported fields from both test matchers, or update the production event builders to emit them consistently and retain the assertions. The test should reflect the actual event contract intended by the service.
Id on Platform: 4055
Category: 🐛 Bug Risk
File: .../service/RagIndexTrackingService.java:75
Legacy transitions silently fail under active ownership
The compatibility overload delegates markIndexingCompleted with expectedActiveJobId set to null, while ownsStatus requires the expected owner to equal the status row's active job ID. Once a status is owned by any non-null job, this overload returns the unchanged status without completing the index. The same delegation pattern exists for the retained overloads of failure, heartbeat, and incremental completion. Any caller still using these public legacy signatures can therefore report successful work while leaving the status stuck in INDEXING or UPDATING.
Also affects: java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/RagIndexTrackingService.java:74
💡 Suggested Fix
Preserve a deliberate compatibility contract for the legacy overloads: either migrate/remove all legacy entry points, or make their behavior explicit rather than passing null through the stale-worker ownership check. If legacy calls are allowed to transition state, add a separate transition path that documents and enforces the appropriate ownership policy.
Id on Platform: 4058
Category: 🐛 Bug Risk
File: .../index_manager/collection_manager.py:144
Mismatched payload schemas are never repaired
The required schema now changes fields such as pr from the previous all-KEYWORD configuration to BOOL, but _ensure_payload_indexes only skips an index when its existing type exactly matches and otherwise calls create_payload_index again. Qdrant does not generally replace an existing payload index when a different schema is requested; it returns a conflict/error instead. As a result, collections created before this change can retain incompatible schemas, the repair is retried on every use, and filters on those fields may remain unsupported or fail.
💡 Suggested Fix
Detect existing indexes with an incompatible schema and explicitly delete/recreate them using the required type, or provide a one-time migration path. Do not mark the collection repaired until all mismatched indexes have been successfully migrated.
Id on Platform: 4059
Category: 🐛 Bug Risk
File: .../qadoc/QaDocContentParser.java:30
Any section six is parsed as environment
The first alternative in ENVIRONMENT_SECTION_HEADING matches any heading beginning with 6., regardless of its title. A document containing a legitimate section such as ## 6. Security Considerations or ## 6. Deployment Risks will have that section removed from overview and exposed as environment instead. This can corrupt parsed QA document content and shareable-section replacement behavior for otherwise valid numbered documents.
💡 Suggested Fix
Remove the broad 6.\s+.+ alternative and match only the supported environment/setup titles. If a numbered legacy environment heading is required, constrain the title after the number to the known environment wording.
Id on Platform: 4060
Category: 🐛 Bug Risk
File: .../analysis/RagIndexStatusRepository.java:55
Recovery marks missing indexes as indexed
recoverAbandonedIncrementalUpdate unconditionally changes an abandoned INDEXING or UPDATING row to INDEXED, but its predicate does not require lastIndexedAt to be non-null. For a first indexing attempt with no completed checkpoint, recovery therefore advertises an index as usable even though no prior index exists. Consumers using isProjectIndexed can then skip required index creation or query a nonexistent collection.
💡 Suggested Fix
Restrict the restore-to-INDEXED update to rows with a usable prior checkpoint, such as r.lastIndexedAt IS NOT NULL. Handle an abandoned first indexing attempt separately by transitioning it to an appropriate failed/unindexed state.
Id on Platform: 4061
Category: 🐛 Bug Risk
File: .../qadoc/QaDocContentParser.java:30
QA content parser misclassifies arbitrary numbered section 6 and corrupts downstream previews
QA content parser misclassifies arbitrary numbered section 6 and corrupts downstream previews
QaDocContentParser treats every heading beginning with 6. as the environment/setup section, regardless of its title. The parser is used both when generating the public-preview content and when resolving that content for anonymous sharing. A valid document containing a section such as ## 6. Security Considerations is therefore split incorrectly: the section is removed from the overview and returned as environment/setup content instead.
Evidence: QaDocPublicPreviewService parses the persisted QA document before issuing a share, while QaDocShareProvider parses the same document when constructing the public response. Both consumers consequently inherit the parser's broad 6. match and can expose or render the wrong sections in the preview.
Business impact: QA guides with legitimate numbered section 6 content can lose that content from the overview and display it under the wrong public-preview section, producing misleading documentation and potentially exposing setup-like content in an unintended response field.
Also affects: java-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocPublicPreviewService.java, java-ecosystem/services/web-server/src/main/java/org/rostilos/codecrow/webserver/publicshare/qadoc/QaDocShareProvider.java
💡 Suggested Fix
Restrict the numbered-section alternative to the intended environment/setup heading, or require the heading text to match the known environment/setup titles. Add an end-to-end test covering a document with ## 6. Security Considerations through both preview generation and public-share resolution.
🔵 Low Severity Issues
Id on Platform: 4056
Category: 🐛 Bug Risk
File: .../cloud/JiraCloudClient.java:671
URLs inside code spans become links
The bare-URL branch runs before the existing inline-code branch. Consequently, Markdown such as `https://example.com` is split into a linked URL node instead of remaining code-formatted text. This changes the documented inline-code behavior and can alter literal command/configuration text when Jira comments are generated.
💡 Suggested Fix
Handle inline-code spans before bare URL detection, or explicitly suppress URL recognition while the parser is inside a code span. Preserve the existing code mark for the complete span.
Id on Platform: 4057
Category: 🛡️ Error Handling
File: .../service/AnalysisJobService.java:96
Null skip reason causes exception
Map.of rejects null keys and values. The public skipJob method accepts an unconstrained String reason, so a caller passing null causes an immediate NullPointerException instead of completing the job as skipped. This makes the new lifecycle helper unsafe for optional or unavailable diagnostic reasons.
💡 Suggested Fix
Use a null-tolerant mutable map, normalize a null reason to an empty/default message, or explicitly validate and reject null with a clear IllegalArgumentException before constructing the result map.
Files Affected
- .../qadoc/QaDocContentParser.java: 2 issues
- .../service/RagIndexTrackingService.java: 1 issue
- .../index_manager/collection_manager.py: 1 issue
- .../cloud/JiraCloudClient.java: 1 issue
- .../analysis/RagIndexStatusRepository.java: 1 issue
- .../branch/BranchAnalysisGateServiceTest.java: 1 issue
- .../service/AnalysisJobService.java: 1 issue
| event -> Long.valueOf(41L).equals(event.get("prNumber")) | ||
| && event.get("message").toString().contains("PR #41"))); | ||
| && event.get("message").toString().contains("PR #41") | ||
| && JobType.BRANCH_ANALYSIS.name().equals( |
There was a problem hiding this comment.
🟡 MEDIUM | Testing
Assertions expect fields absent from wait events
The updated matcher requires waitingJobType and blockingJobType to be present in the emitted event. However, the current BranchAnalysisGateService.emitWait and emitBranchWait implementations populate the event with type, state, message, branch name, waited time, and optionally PR number, but do not add either of these fields. Consequently, mergeWaitsOnlyForNewestAttemptOfItsOwnPr and the corresponding branch-wait test will fail whenever a wait event is emitted.
💡 Suggested fix
Either remove the two unsupported fields from both test matchers, or update the production event builders to emit them consistently and retain the assertions. The test should reflect the actual event contract intended by the service.
| Integer filesIndexed, Integer chunkCount) { | ||
| RagIndexStatus status = ragIndexStatusRepository.findByProjectId(project.getId()) | ||
| return markIndexingCompleted( | ||
| project, branchName, commitHash, filesIndexed, chunkCount, null); |
There was a problem hiding this comment.
🟡 MEDIUM | Bug Risk
Legacy transitions silently fail under active ownership
The compatibility overload delegates markIndexingCompleted with expectedActiveJobId set to null, while ownsStatus requires the expected owner to equal the status row's active job ID. Once a status is owned by any non-null job, this overload returns the unchanged status without completing the index. The same delegation pattern exists for the retained overloads of failure, heartbeat, and incremental completion. Any caller still using these public legacy signatures can therefore report successful work while leaving the status stuck in INDEXING or UPDATING.
Also affects: java-ecosystem/libs/rag-engine/src/main/java/org/rostilos/codecrow/ragengine/service/RagIndexTrackingService.java:74
💡 Suggested fix
Preserve a deliberate compatibility contract for the legacy overloads: either migrate/remove all legacy entry points, or make their behavior explicit rather than passing null through the stale-worker ownership check. If legacy calls are allowed to transition state, add a separate transition path that documents and enforces the appropriate ownership policy.
| // Jira does not auto-link plain text inside an ADF document. Add | ||
| // the link mark explicitly, preserving fragments used by opaque | ||
| // public-share URLs. | ||
| if (text.startsWith("https://", pos) || text.startsWith("http://", pos)) { |
There was a problem hiding this comment.
🔵 LOW | Bug Risk
URLs inside code spans become links
The bare-URL branch runs before the existing inline-code branch. Consequently, Markdown such as `https://example.com` is split into a linked URL node instead of remaining code-formatted text. This changes the documented inline-code behavior and can alter literal command/configuration text when Jira comments are generated.
💡 Suggested fix
Handle inline-code spans before bare URL detection, or explicitly suppress URL recognition while the parser is inside a code span. Preserve the existing code mark for the complete span.
|
|
||
| /** Finish an intentionally skipped job without representing it as a failure. */ | ||
| default void skipJob(Job job, String reason) { | ||
| completeJob(job, Map.of("status", "skipped", "reason", reason)); |
There was a problem hiding this comment.
🔵 LOW | Error Handling
Null skip reason causes exception
Map.of rejects null keys and values. The public skipJob method accepts an unconstrained String reason, so a caller passing null causes an immediate NullPointerException instead of completing the job as skipped. This makes the new lifecycle helper unsafe for optional or unavailable diagnostic reasons.
💡 Suggested fix
Use a null-tolerant mutable map, normalize a null reason to an empty/default message, or explicitly validate and reject null with a clear IllegalArgumentException before constructing the result map.
| ("architecture_group", PayloadSchemaType.KEYWORD), | ||
| ("snapshot_plugin", PayloadSchemaType.KEYWORD), | ||
| ("snapshot_kind", PayloadSchemaType.KEYWORD), | ||
| ("pr", PayloadSchemaType.BOOL), |
There was a problem hiding this comment.
🟡 MEDIUM | Bug Risk
Mismatched payload schemas are never repaired
The required schema now changes fields such as pr from the previous all-KEYWORD configuration to BOOL, but _ensure_payload_indexes only skips an index when its existing type exactly matches and otherwise calls create_payload_index again. Qdrant does not generally replace an existing payload index when a different schema is requested; it returns a conflict/error instead. As a result, collections created before this change can retain incompatible schemas, the repair is retried on every use, and filters on those fields may remain unsupported or fail.
💡 Suggested fix
Detect existing indexes with an incompatible schema and explicitly delete/recreate them using the required type, or provide a one-time migration path. Do not mark the collection repaired until all mismatched indexes have been successfully migrated.
| Pattern.CASE_INSENSITIVE | ||
| ); | ||
| private static final Pattern ENVIRONMENT_SECTION_HEADING = Pattern.compile( | ||
| "^(?:6\\.\\s+.+|(?:\\d+\\.\\s*)?(?:Environment and Setup Notes|" |
There was a problem hiding this comment.
🟡 MEDIUM | Bug Risk
Any section six is parsed as environment
The first alternative in ENVIRONMENT_SECTION_HEADING matches any heading beginning with 6., regardless of its title. A document containing a legitimate section such as ## 6. Security Considerations or ## 6. Deployment Risks will have that section removed from overview and exposed as environment instead. This can corrupt parsed QA document content and shareable-section replacement behavior for otherwise valid numbered documents.
💡 Suggested fix
Remove the broad 6.\s+.+ alternative and match only the supported environment/setup titles. If a numbered legacy environment heading is required, constrain the title after the number to the known environment wording.
| "r.errorMessage = CONCAT('Incremental update failed: ', :errorMessage), " + | ||
| "r.failedIncrementalCount = COALESCE(r.failedIncrementalCount, 0) + 1, " + | ||
| "r.activeJobId = NULL " + | ||
| "WHERE r.project.id = :projectId AND r.activeJobId = :jobId " + |
There was a problem hiding this comment.
🟡 MEDIUM | Bug Risk
Recovery marks missing indexes as indexed
recoverAbandonedIncrementalUpdate unconditionally changes an abandoned INDEXING or UPDATING row to INDEXED, but its predicate does not require lastIndexedAt to be non-null. For a first indexing attempt with no completed checkpoint, recovery therefore advertises an index as usable even though no prior index exists. Consumers using isProjectIndexed can then skip required index creation or query a nonexistent collection.
💡 Suggested fix
Restrict the restore-to-INDEXED update to rows with a usable prior checkpoint, such as r.lastIndexedAt IS NOT NULL. Handle an abandoned first indexing attempt separately by transitioning it to an appropriate failed/unindexed state.
| Pattern.CASE_INSENSITIVE | ||
| ); | ||
| private static final Pattern ENVIRONMENT_SECTION_HEADING = Pattern.compile( | ||
| "^(?:6\\.\\s+.+|(?:\\d+\\.\\s*)?(?:Environment and Setup Notes|" |
There was a problem hiding this comment.
🟡 MEDIUM | Bug Risk
QA content parser misclassifies arbitrary numbered section 6 and corrupts downstream previews
QA content parser misclassifies arbitrary numbered section 6 and corrupts downstream previews
QaDocContentParser treats every heading beginning with 6. as the environment/setup section, regardless of its title. The parser is used both when generating the public-preview content and when resolving that content for anonymous sharing. A valid document containing a section such as ## 6. Security Considerations is therefore split incorrectly: the section is removed from the overview and returned as environment/setup content instead.
Evidence: QaDocPublicPreviewService parses the persisted QA document before issuing a share, while QaDocShareProvider parses the same document when constructing the public response. Both consumers consequently inherit the parser's broad 6. match and can expose or render the wrong sections in the preview.
Business impact: QA guides with legitimate numbered section 6 content can lose that content from the overview and display it under the wrong public-preview section, producing misleading documentation and potentially exposing setup-like content in an unintended response field.
Also affects: java-ecosystem/services/pipeline-agent/src/main/java/org/rostilos/codecrow/pipelineagent/qadoc/QaDocPublicPreviewService.java, java-ecosystem/services/web-server/src/main/java/org/rostilos/codecrow/webserver/publicshare/qadoc/QaDocShareProvider.java
💡 Suggested fix
Restrict the numbered-section alternative to the intended environment/setup heading, or require the heading text to match the known environment/setup titles. Add an end-to-end test covering a document with ## 6. Security Considerations through both preview generation and public-share resolution.
1 participant
feat: add QA test case sharing and harden RAG recovery
feat(qa-doc): add secure shared QA document previews
fix(qa-doc): replace Jira environment details with preview link
fix(review): resolve analysis recovery and QA documentation gaps
fix(analysis): harden RAG recovery and QA handoff ownership
Summary by CodeRabbit
New Features
Bug Fixes