fix: Improve SSE response and add error handling

[JayGhiya]

User description

This commit includes the following changes:

1. Simplify the SSE response headers by removing unnecessary headers.
2. Refactor the main.py file to improve error handling and provide better progress reporting during the codebase detection process.
3. Remove the duration_seconds field from the DetectionResult model, as it is no longer needed.
4. Modify the PythonRipgrepDetector to use asyncio.to_thread for cloning the repository, improving the overall performance.

The main goal of these changes is to enhance the stability and reliability of the codebase detection process, providing better error handling and progress reporting to the client.

---

PR Type

Bug fix, Enhancement

---

Description

• Improve SSE response headers and error handling

• Remove duration tracking from detection results

• Optimize repository cloning with async threading

• Reorganize imports and fix code structure

---

Diagram Walkthrough

flowchart LR
  A["SSE Request"] --> B["Generate Events"]
  B --> C["Clone Repository"]
  C --> D["Detect Codebases"]
  D --> E["Stream Results"]
  C -- "asyncio.to_thread" --> F["Async Clone"]
  B -- "Simplified Headers" --> G["EventSourceResponse"]

Loading

File Walkthrough

Relevant files

Enhancement

main.py Refactor SSE handling and remove duration tracking              unoplat-code-confluence-ingestion/code-confluence-flow-bridge/src/code_confluence_flow_bridge/main.py Reorganized imports to top of file for better structure Simplified SSE response headers by using EventSourceResponse Removed duration tracking from detection process Fixed missing commit statement in GitHub issue creation +39/-54  progress_models.py Remove duration field from detection result                            unoplat-code-confluence-ingestion/code-confluence-flow-bridge/src/code_confluence_flow_bridge/parser/package_manager/detectors/progress_models.py Removed duration_seconds field from DetectionResult model +0/-1      python_ripgrep_detector.py Optimize async repository cloning performance                        unoplat-code-confluence-ingestion/code-confluence-flow-bridge/src/code_confluence_flow_bridge/parser/package_manager/detectors/python_ripgrep_detector.py Reorganized imports to follow proper structure Modified repository cloning to use asyncio.to_thread for async execution +11/-10  sse_response.py Simplify SSE response headers                                                        unoplat-code-confluence-ingestion/code-confluence-flow-bridge/src/code_confluence_flow_bridge/parser/package_manager/detectors/sse_response.py Simplified default SSE headers by removing cache control and connection headers Kept only essential Content-Type header for SSE responses +0/-3

---

[huly-for-github]

Connected to Huly®: UNOPL-817

[claude]

Claude finished @JayGhiya's task —— View job

---

PR Review Analysis

I've analyzed all four changed files in this pull request. Here's my comprehensive review focusing on code quality, potential bugs, performance considerations, security concerns, and test coverage.

Todo List

• Read and analyze main.py changes
• Review progress_models.py modifications
• Examine python_ripgrep_detector.py updates
• Check sse_response.py changes
• Provide comprehensive review feedback

---

Review Findings

✅ Positive Changes

1. Better Error Handling: The main.py changes improve error handling in the GitHub issue creation endpoint by adding a missing await session.commit() call at line 1911.

2. Performance Improvement: Using asyncio.to_thread() in python_ripgrep_detector.py:136 is a good practice for wrapping synchronous operations (Git cloning) in async contexts without blocking the event loop.

3. Code Organization: Moving imports to the top of files improves readability and follows Python conventions.

4. Simplified SSE Headers: Removing unnecessary cache-control and connection headers from sse_response.py keeps the implementation cleaner.

⚠️ Areas of Concern

1. Potential Bug - Missing Commit Statement

File: main.py:1911

await session.commit()

While this fixes the missing commit, there's no transaction rollback handling if the commit fails. Consider wrapping in a try-catch block.

2. Memory Management Issue

File: python_ripgrep_detector.py:136

repo_path = await asyncio.to_thread(self._clone_repository, git_url, github_token)

The _clone_repository method creates local directories and clones repositories, but there's no cleanup mechanism for failed operations or temporary files. This could lead to disk space issues over time.

3. Security Concern - Token Exposure

File: python_ripgrep_detector.py:325-330

if git_url.startswith("https://"):
    clone_url: str = git_url.replace("https://", f"https://{github_token}@")

The GitHub token is embedded directly in the URL, which could be logged or exposed in error messages. Consider using Git credential helpers or environment variables instead.

4. Missing Error Context

File: main.py:382-481
The generate_sse_events function catches all exceptions but only logs the error message. Stack traces and detailed error context are lost, making debugging harder.

🔄 Performance Considerations

1. Async Repository Cloning: The change to use asyncio.to_thread() is good, but consider implementing a repository cache to avoid repeated clones of the same repository.

2. SSE Connection Overhead: The simplified headers reduce response size, but consider implementing connection pooling for high-traffic scenarios.

🧪 Test Coverage Concerns

1. Missing Test Cases: The changes don't include tests for:

   • The async repository cloning functionality
   • Error handling in SSE event generation
   • The removed duration tracking functionality

2. Integration Testing: The SSE endpoint changes should have integration tests to ensure the streaming still works correctly after header modifications.

🏗️ Code Quality Suggestions

1. Type Hints: Some methods in python_ripgrep_detector.py could benefit from more specific return type hints, especially _clone_repository().

2. Constants: Magic strings like "unoplat-code-confluence-repository-context-ingestion" should be defined as constants.

3. Documentation: The removed duration_seconds field from DetectionResult should be documented in the commit or CHANGELOG to explain why it was removed.

🎯 Recommendations

1. Add transaction rollback for the database commit operation
2. Implement cleanup mechanisms for cloned repositories
3. Use secure credential handling for GitHub tokens in Git operations
4. Add comprehensive error logging with stack traces
5. Include unit tests for the new async functionality
6. Document the breaking change of removing duration tracking

⭐ Overall Assessment

This PR makes solid improvements to SSE response handling and async performance, but has some areas that need attention before merging:

• Code Quality: Good (7/10)
• Security: Needs Improvement (6/10) - token handling concern
• Performance: Good (8/10) - async improvements
• Test Coverage: Needs Improvement (5/10) - missing tests
• Documentation: Needs Improvement (6/10) - breaking changes undocumented

---

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Missing Commit The database session commit statement was removed from the GitHub issue creation function, which means changes to the database won't be persisted. This could lead to data loss or inconsistent state. return issue_tracking_full Reduced Headers Removing cache control and connection headers from SSE responses may cause issues with proxy servers, CDNs, or browsers that rely on these headers for proper streaming behavior. The removed headers were specifically designed to prevent buffering and ensure real-time streaming. default_headers = { "Content-Type": "text/event-stream; charset=utf-8", } Async Threading Using asyncio.to_thread for repository cloning may not provide the expected performance benefits since git operations are typically I/O bound and the original synchronous approach might be more appropriate. The change should be validated for actual performance improvement. repo_path = await asyncio.to_thread(self._clone_repository, git_url, github_token)

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Missing database commit statement The PR removes the await session.commit() statement from the GitHub issue creation endpoint. This will cause database changes to not be persisted, breaking the functionality of issue tracking. Examples: unoplat-code-confluence-ingestion/code-confluence-flow-bridge/src/code_confluence_flow_bridge/main.py [1909-1911] repo_run.issue_tracking = issue_data return issue_tracking_full Solution Walkthrough: Before: async def create_github_issue(..., session: AsyncSession): try: ... if request.parent_workflow_run_id: ... repo_run.issue_tracking = issue_data # Missing session.commit() return issue_tracking_full except Exception as e: ... After: async def create_github_issue(..., session: AsyncSession): try: ... if request.parent_workflow_run_id: ... repo_run.issue_tracking = issue_data await session.commit() # Persist changes return issue_tracking_full except Exception as e: ... Suggestion importance[1-10]: 10 __ Why: This suggestion correctly identifies a critical bug where a removed await\ \ session.commit() call prevents database updates, breaking the core functionality\ \ of the create_github_issue endpoint.	High
Update

[claude]

Claude finished @JayGhiya's task —— View job

---

PR Review Analysis

I've analyzed all four changed files in this pull request. Here's my comprehensive review focusing on code quality, potential bugs, performance considerations, security concerns, and test coverage.

Todo List

• Read and analyze main.py changes
• Review progress_models.py modifications
• Examine python_ripgrep_detector.py updates
• Check sse_response.py changes
• Analyze failing test cases and root cause
• Provide comprehensive review feedback

---

Review Findings

✅ Positive Changes

1. Better Error Handling: The main.py changes improve error handling in the GitHub issue creation endpoint by adding a missing await session.commit() call at line 1911.

2. Performance Improvement: Using asyncio.to_thread() in python_ripgrep_detector.py:136 is a good practice for wrapping synchronous operations (Git cloning) in async contexts without blocking the event loop.

3. Code Organization: Moving imports to the top of files improves readability and follows Python conventions.

4. Simplified SSE Headers: Removing unnecessary cache-control and connection headers from sse_response.py keeps the implementation cleaner.

⚠️ Areas of Concern

1. Critical Issue - Test Failures (Missing Headers)

Root Cause: The failing tests are expecting cache-control headers that were removed in sse_response.py:45-47.

Files affected:

• tests/integration/test_start_ingestion.py:217
• tests/integration/test_delete_repository.py:176
• tests/parser/package_manager/detectors/test_detect_codebases_sse.py:222

Issue: Tests assert response.headers["cache-control"] == "no-cache" but the header was removed from the default headers in EventSourceResponse.

2. Potential Bug - Missing Commit Statement

File: main.py:1911

await session.commit()

While this fixes the missing commit, there's no transaction rollback handling if the commit fails. Consider wrapping in a try-catch block.

3. Memory Management Issue

File: python_ripgrep_detector.py:136

repo_path = await asyncio.to_thread(self._clone_repository, git_url, github_token)

The _clone_repository method creates local directories and clones repositories, but there's no cleanup mechanism for failed operations or temporary files. This could lead to disk space issues over time.

4. Security Concern - Token Exposure

File: python_ripgrep_detector.py:325-330

if git_url.startswith("https://"):
    clone_url: str = git_url.replace("https://", f"https://{github_token}@")

The GitHub token is embedded directly in the URL, which could be logged or exposed in error messages. Consider using Git credential helpers or environment variables instead.

5. Missing Error Context

File: main.py:382-481
The generate_sse_events function catches all exceptions but only logs the error message. Stack traces and detailed error context are lost, making debugging harder.

🔄 Performance Considerations

1. Async Repository Cloning: The change to use asyncio.to_thread() is good, but consider implementing a repository cache to avoid repeated clones of the same repository.

2. SSE Connection Overhead: The simplified headers reduce response size, but consider implementing connection pooling for high-traffic scenarios.

🧪 Test Coverage Concerns

1. Missing Test Cases: The changes don't include tests for:

   • The async repository cloning functionality
   • Error handling in SSE event generation
   • The removed duration tracking functionality

2. Integration Testing: The SSE endpoint changes should have integration tests to ensure the streaming still works correctly after header modifications.

🏗️ Code Quality Suggestions

1. Type Hints: Some methods in python_ripgrep_detector.py could benefit from more specific return type hints, especially _clone_repository().

2. Constants: Magic strings like "unoplat-code-confluence-repository-context-ingestion" should be defined as constants.

3. Documentation: The removed duration_seconds field from DetectionResult should be documented in the commit or CHANGELOG to explain why it was removed.

🎯 Recommendations

1. Fix failing tests by updating the test assertions to match the new SSE header structure
2. Add transaction rollback for the database commit operation
3. Implement cleanup mechanisms for cloned repositories
4. Use secure credential handling for GitHub tokens in Git operations
5. Add comprehensive error logging with stack traces
6. Include unit tests for the new async functionality
7. Document the breaking change of removing duration tracking

⭐ Overall Assessment

This PR makes solid improvements to SSE response handling and async performance, but has some areas that need attention before merging:

• Code Quality: Good (7/10)
• Security: Needs Improvement (6/10) - token handling concern
• Performance: Good (8/10) - async improvements
• Test Coverage: Needs Improvement (5/10) - missing tests
• Documentation: Needs Improvement (6/10) - breaking changes undocumented

---