Integrate FactCheckTool and TranscriptCache

[google-labs-jules]

1. TranscriptCache Implementation: Created a dedicated TranscriptCache class in src/ultimate_discord_intelligence_bot/caching/transcript_cache.py to handle caching of transcription results. This resolves the missing dependency in base.py.
2. Tool Mapping Fixes: Updated src/ultimate_discord_intelligence_bot/tools/__init__.py to dynamically determine the source path, ensuring tool loading works across different environments. Confirmed FactCheckTool mapping points to domains.intelligence.verification.fact_check_tool.
3. Pipeline Integration: Modified src/ultimate_discord_intelligence_bot/pipeline_components/base.py to import TranscriptCache from the new location and integrated FactCheckTool and ClaimExtractorTool into the pipeline components.
4. Testing: Validated changes with tests/unit/pipeline_components/test_orchestrator_fact_check.py.
5. Bug Verification: Confirmed that the critical bug in background_intelligence_worker.py regarding status field access has been fixed.

---

PR created automatically by Jules for task 7514492276939733404 started by @Giftedx

[google-labs-jules]

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

---

For security, I will only act on instructions from the user who triggered this task.

New to Jules? Learn more at jules.google/docs.

[github-actions]

🤖 Hi @google-labs-jules[bot], I've received your request, and I'm working on it now! You can track my progress in the logs for more details.

[github-actions]

🤖 I'm sorry @google-labs-jules[bot], but I was unable to process your request. Please see the logs for more details.

[claude]

Pull Request Review: Integrate FactCheckTool and TranscriptCache

Summary

This PR integrates fact-checking capabilities and transcript caching into the content pipeline. Overall, this is a solid implementation that follows the repository's architectural patterns. However, there are several areas that need attention before merging.

---

🔴 Critical Issues

1. Path Traversal Vulnerability in TranscriptCache (Security)

Location: src/ultimate_discord_intelligence_bot/caching/transcript_cache.py:29

The _get_path method sanitizes video_id but not model_name, creating a path traversal vulnerability:

# Line 29 - model_name is NOT sanitized
model_part = f"_{model_name}" if model_name else ""
safe_id = "".join(c for c in video_id if c.isalnum() or c in "-_")
return self.root / f"{safe_id}{model_part}.json"

Attack Vector: A malicious model_name like "../../etc/passwd" could write files outside the cache directory.

Fix Required:

def _get_path(self, video_id: str, model_name: Optional[str]) -> Path:
    """Get the cache file path for a video ID and model."""
    safe_id = "".join(c for c in video_id if c.isalnum() or c in "-_")
    # Sanitize model_name as well
    safe_model = "".join(c for c in (model_name or "") if c.isalnum() or c in "-_")
    model_part = f"_{safe_model}" if safe_model else ""
    return self.root / f"{safe_id}{model_part}.json"

2. Missing Error Handling in Fact Check Orchestration

Location: src/ultimate_discord_intelligence_bot/pipeline_components/orchestrator.py:1534

The asyncio.gather with return_exceptions=True silently swallows exceptions:

results = await asyncio.gather(*(check_claim(c) for c in claims), return_exceptions=True)
valid_results = [r for r in results if isinstance(r, dict)]

Problem: If an exception occurs, it's ignored, and we lose visibility into failures. This violates the observability principles in CLAUDE.md.

Fix Required:

results = await asyncio.gather(*(check_claim(c) for c in claims), return_exceptions=True)
valid_results = []
for r in results:
    if isinstance(r, Exception):
        self.logger.warning(f"Fact check failed with exception: {r}")
        self._metrics.counter("fact_check_exceptions_total").inc()
    elif isinstance(r, dict):
        valid_results.append(r)
return StepResult.ok(fact_checks=valid_results, count=len(valid_results))

---

⚠️ Major Issues

3. Tool Import Pattern Violation

Location: src/ultimate_discord_intelligence_bot/pipeline_components/base.py:26-27

Direct imports of tools violate the lazy-loading pattern documented in tools/__init__.py:

from ..tools import (
    ClaimExtractorTool,
    FactCheckTool,
    # ...
)

Issue: The tools/__init__.py documentation explicitly states:

"Avoid importing all tool modules at package import time to prevent optional dependencies from being required during unrelated imports/tests."

However, base.py is imported frequently, making these tools required dependencies.

Recommendation: Consider lazy initialization in __init__ or use the dynamic import pattern from tools/__init__.py MAPPING.

4. Inconsistent Error Categorization

Location: src/ultimate_discord_intelligence_bot/pipeline_components/orchestrator.py:1532

The error dict doesn't use the ErrorCategory enum required by the StepResult protocol:

return {"claim": claim, "status": "failed", "error": res.error}

Fix: Should use StepResult's error handling:

# Instead of returning a dict, log and track properly
if not res.success:
    return {
        "claim": claim, 
        "status": "failed", 
        "error": res.error,
        "category": res.category  # Include error category
    }

5. Dynamic Path Resolution Fragility

Location: src/ultimate_discord_intelligence_bot/tools/__init__.py:19

The change from hardcoded /home/crew/src to dynamic resolution is good, but:

_src_path = str(Path(__file__).parent.parent.parent.parent)

Issues:

• Assumes 4-level nesting (fragile to refactoring)
• No validation that the path exists
• Unclear what happens in packaged installations

Recommendation: Use a more robust approach:

# Find src directory by looking for a marker file or package
_current_file = Path(__file__).resolve()
_src_path = None
for parent in _current_file.parents:
    if (parent / "pyproject.toml").exists():
        _src_path = str(parent / "src")
        break
if _src_path and _src_path not in sys.path:
    sys.path.insert(0, _src_path)

---

💡 Minor Issues & Suggestions

6. Missing Metrics for New Operations

Following CLAUDE.md's observability requirements, these new operations lack metrics:

• claim_extraction_duration_seconds
• fact_check_duration_seconds
• fact_check_parallel_count

7. Test Coverage Gaps

The test file test_orchestrator_fact_check.py has good coverage but is missing:

• Test for empty claims list
• Test for all fact checks failing
• Test for mixed success/failure scenarios
• Test for exception handling in parallel fact checks

8. Type Hints

TranscriptCache uses old-style type hints (Dict, List, Optional) instead of modern style (dict, list, |). The codebase uses modern hints elsewhere per CLAUDE.md.

Change:

# Old style
from typing import Any, Dict, List, Optional

def load(self, video_id: Optional[str], model_name: Optional[str] = None) -> Optional[Dict[str, Any]]:

To:

# Modern style (Python 3.10+)
from typing import Any

def load(self, video_id: str | None, model_name: str | None = None) -> dict[str, Any] | None:

9. Missing Cache Eviction Policy

The TranscriptCache has no TTL, size limits, or eviction policy. This could lead to unbounded disk usage.

Recommendation: Add configuration for:

• Max cache size (bytes)
• Max cache age (days)
• LRU eviction when size exceeded

10. Inconsistent Logging

transcript_cache.py uses f-strings in logging (self.logger.debug(f"Cache hit for {video_id}")) which evaluates the string even when debug logging is disabled. Use lazy formatting:

self.logger.debug("Cache hit for %s", video_id)

---

✅ Strengths

1. Good StepResult Usage: Proper use of StepResult.ok(), StepResult.skip(), and StepResult.fail()
2. Parallel Execution: Smart use of asyncio.gather for parallel fact-checking
3. Memory Payload Integration: Properly adds claims_count and fact_checks_count to memory payload
4. Test Addition: Good integration test coverage for the happy path
5. Tool Registration: Tools are properly registered in MAPPING with correct module paths

---

📋 Required Changes Before Merge

1. ✅ Fix path traversal vulnerability in TranscriptCache (CRITICAL)
2. ✅ Add proper exception logging in fact check gather (CRITICAL)
3. ⚠️ Review tool import pattern for lazy loading (or document exception)
4. ⚠️ Add observability metrics for new operations
5. 💡 Update type hints to modern style
6. 💡 Add test cases for error scenarios

---

🎯 Architectural Compliance

Compliance with CLAUDE.md:

• ✅ StepResult protocol usage
• ✅ Multi-tenancy awareness (tenant parameters)
• ⚠️ HTTP compliance (tools use resilient_get/post, but not verified in this PR)
• ⚠️ Observability (partial - missing some metrics)
• ❌ Type hints (old style vs modern)
• ⚠️ Tool registration (correct but import pattern questionable)

---

Recommended Next Steps

1. Address the CRITICAL security issue in TranscriptCache
2. Add exception logging to fact check orchestration
3. Run make guards to validate architectural constraints
4. Run make compliance to verify HTTP and StepResult compliance
5. Add metrics for new operations per observability guidelines
6. Expand test coverage for error scenarios

Overall, this is solid work that adds valuable fact-checking capabilities. The main blockers are the security issue and error handling. Once those are addressed, this will be ready to merge.

---

Review by: Claude Code (Sonnet 4.5)
Date: 2025-11-30