Cache Consolidation and Test Health Improvements

[google-labs-jules]

This PR addresses "Phase 1: Quick Wins" from the synthesis plan, focusing on cache consolidation, deprecation cleanup, and test health.

Changes:

1. Cache Consolidation (Phase 3 & 4):

   • Added src/platform/cache/shadow/harness.py to enable A/B testing between legacy and unified cache implementations.
   • Migrated src/ultimate_discord_intelligence_bot/tools/observability/unified_cache_tool.py to use the UnifiedCacheService facade, removing dependency on legacy cache logic.
   • Added metrics instrumentation to migrated tools.

2. Deprecation Cleanup (P3.3):

   • Added DeprecationWarning to src/ai/routing/linucb_router.py and src/ai/routing/bandit_router.py, directing users to the new plugins module.
   • Added warning to UnifiedCacheService when initialized with legacy config.

3. Test Fixes (Phase 5 Start):

   • Personality Manager Tests: Fixed TypeErrors and AttributeErrors in tests/unit/test_personality_manager.py by updating PersonalityTraits (added unsafe_hash, clamping), PersonalityMetrics (added missing fields), and fixing method calls in PersonalityStateManager tests.
   • Message Evaluator Tests: Fixed TypeErrors and AttributeErrors in tests/unit/test_message_evaluator.py by updating MessageContext (added relevant_memories), EvaluationResult (added estimated_cost), and fixing service mocks.

Verification:

• tests/fast/test_fast.py passes.
• tests/unit/test_personality_manager.py passes 21/26 tests (critical compilation/runtime errors fixed, remaining failures are due to StepResult wrapping logic which requires deeper refactoring).
• tests/unit/test_message_evaluator.py passes 26/31 tests (critical compilation/runtime errors fixed).
• Compliance scripts (validate_tools_exports.py, metrics_instrumentation_guard.py) passed.

---

PR created automatically by Jules for task 10588732889125479474 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 - Cache Consolidation and Test Health Improvements

Thank you for this comprehensive PR! I've conducted a thorough review focusing on code quality, architecture, security, and performance. Here's my detailed feedback:

---

✅ Strengths

1. Cache Consolidation Architecture

• Shadow Harness Pattern (src/platform/cache/shadow/harness.py): Excellent approach for A/B testing cache implementations. The shadow mode allows safe migration with real-time comparison.
• Proper Metrics Instrumentation: Good use of get_metrics().counter() calls in unified_cache_tool.py (lines 62, 103, 124)
• Clean Migration Path: UnifiedCacheTool properly uses UnifiedCacheService facade, eliminating legacy dependencies

2. Deprecation Strategy

• Clear User Communication: Deprecation warnings in linucb_router.py and bandit_router.py with proper stacklevel=2 for accurate call site reporting
• Graceful Degradation: Legacy modules still function while warning users to migrate

3. Test Health Improvements

• Critical Fixes: Resolved TypeErrors and AttributeErrors that were blocking test execution
• Dataclass Enhancements: Added unsafe_hash=True to PersonalityTraits for hashability, proper field defaults

---

⚠️ Issues & Recommendations

HIGH PRIORITY

1. Security: Exception Information Leakage

Location: src/discord_modules_old_shadowing_package/message_evaluator.py:79

return StepResult.fail(f"Cache status failed: {e}")  # Line 135 unified_cache_tool.py
return StepResult.fail(f"Message evaluation failed: {e\!s}")  # Line 209

Issue: Raw exception details exposed in error messages may leak internal implementation details or sensitive paths.

Recommendation:

logger.error(f"Cache status failed: {e}", exc_info=True)
return StepResult.fail("Cache status retrieval failed", error_context={"error_type": type(e).__name__})

2. Missing Error Categorization in StepResult Calls

Location: Multiple files

According to CLAUDE.md, StepResult errors should include proper categorization:

return StepResult.error("Category", "Message", context={...})

Current usage:

return StepResult.fail(f"Value is required for set operation")  # No category

Recommendation: Add error categories like "ValidationError", "CacheError", "ConfigurationError" per the StepResult protocol.

3. Race Condition in PersonalityTraits Clamping

Location: src/discord_modules_old_shadowing_package/personality/personality_manager.py:36-45

def __post_init__(self):
    """Clamp traits to [0.0, 1.0] range."""
    self.humor = self._clamp(self.humor)
    # ... multiple assignments

Issue: With unsafe_hash=True, modifying fields in __post_init__ after hashing can cause hash inconsistency if instance is already in a set/dict.

Recommendation: Use field(default_factory=...) with validator or make clamping happen before initialization:

@dataclass(frozen=True)  # Consider frozen for hashable dataclasses
class PersonalityTraits:
    humor: float = field(default=0.8)
    
    def __post_init__(self):
        object.__setattr__(self, 'humor', max(0.0, min(1.0, self.humor)))

---

MEDIUM PRIORITY

4. Memory Leak Risk in Shadow Harness

Location: src/platform/cache/shadow/harness.py:51

self.comparisons: list[ShadowComparison] = []

Issue: Unbounded list growth in long-running processes. With high cache traffic, this could consume significant memory.

Recommendation: Implement a circular buffer or periodic cleanup:

from collections import deque
self.comparisons: deque[ShadowComparison] = deque(maxlen=10000)

5. Asyncio Event Loop Handling Issue

Location: src/ultimate_discord_intelligence_bot/tools/observability/unified_cache_tool.py:81-88

def _run_async(self, coroutine):
    try:
        return asyncio.run(coroutine)
    except RuntimeError:
        loop = asyncio.get_event_loop()
        return loop.run_until_complete(coroutine)

Issue:

• asyncio.run() creates a new event loop, which is expensive
• Catching RuntimeError to get existing loop is a code smell
• May not work correctly in all contexts (e.g., nested async)

Recommendation: Use asyncio.get_event_loop().run_until_complete() directly or make tool async-native:

try:
    loop = asyncio.get_running_loop()
    # Create task in running loop
    return asyncio.ensure_future(coroutine)
except RuntimeError:
    # No running loop, safe to create new one
    return asyncio.run(coroutine)

6. Inconsistent Defaults in MessageContext

Location: src/discord_modules_old_shadowing_package/message_evaluator.py:30-44

direct_mention: bool = False # Alias for is_direct_mention for compat

Issue: Maintaining two aliases (is_direct_mention and direct_mention) with sync logic in __post_init__ adds complexity and potential bugs.

Recommendation: Use @property for backward compatibility:

@property
def direct_mention(self) -> bool:
    return self.is_direct_mention

7. Missing Type Validation

Location: src/discord_modules_old_shadowing_package/personality/personality_manager.py:106-108

user_history: list[float] | np.ndarray

Issue: Type union makes downstream code brittle, requiring isinstance checks everywhere.

Recommendation: Normalize at initialization:

@dataclass
class PersonalityContext:
    _user_history: np.ndarray = field(default_factory=lambda: np.array([]))
    
    @property
    def user_history(self) -> np.ndarray:
        return self._user_history
    
    @user_history.setter
    def user_history(self, value: list[float] | np.ndarray):
        self._user_history = np.asarray(value)

---

LOW PRIORITY

8. Overly Broad Exception Handling

Location: Multiple files (e.g., message_evaluator.py:128)

except Exception as e:
    logger.warning(f"Failed to fetch relevant memories: {e}")

Recommendation: Catch specific exceptions to avoid masking unexpected errors.

9. Deprecated Method Call

Location: src/discord_modules_old_shadowing_package/message_evaluator.py:404

from discord.message_evaluator import MessageContext

Changed to:

from discord_modules_old_shadowing_package.message_evaluator import MessageContext

Note: Ensure all import paths are updated consistently. Run make guards to validate.

---

🧪 Test Coverage Concerns

1. Partial Test Fixes

• test_personality_manager.py: 21/26 passing (5 failures remaining)
• test_message_evaluator.py: 26/31 passing (5 failures remaining)

Recommendation: The PR description acknowledges remaining failures due to "StepResult wrapping logic." Consider:

1. Adding TODO comments in failing tests with issue links
2. Creating follow-up issues for the 10 remaining test failures
3. Ensuring critical paths are covered by passing tests

2. Shadow Harness Testing

Missing tests for:

• CacheShadowHarness.get_stats() edge cases (empty comparisons, division by zero)
• Value comparison edge cases (_compare_values with complex objects)

Recommendation: Add unit tests for the shadow harness before enabling in production.

---

🚀 Performance Considerations

1. Double Cache Writes in Shadow Mode

Location: harness.py:106-144

Every set() operation writes to both legacy and unified caches. This doubles write latency and could impact high-throughput scenarios.

Recommendation:

• Use fire-and-forget for shadow writes: asyncio.create_task()
• Add feature flag to disable shadow mode after confidence is established
• Monitor unified_latency_ms vs legacy_latency_ms metrics

2. Synchronous Logging in Hot Path

Location: Multiple logger.info() calls in cache operations

Recommendation: Use async logging or reduce log verbosity in production.

---

📋 Compliance Checklist

✅ StepResult Protocol: Most tools use StepResult, but need categorization
✅ HTTP Compliance: No HTTP calls in this PR
✅ Tool Registration: Tools exported in __all__
⚠️ Multi-Tenancy: Shadow harness properly passes tenant_id, but could validate tenant isolation in tests
✅ Feature Flags: Deprecation warnings act as feature gates

---

🎯 Action Items

Before Merge:

1. Fix security issue: Sanitize exception messages in error responses
2. Add error categories to StepResult.fail() calls
3. Address unsafe_hash=True + __post_init__ mutation concern
4. Add max size to comparisons list in shadow harness
5. Run make guards and make compliance to validate

Follow-up PRs:

1. Resolve remaining 10 test failures (create tracking issues)
2. Add comprehensive shadow harness tests
3. Implement async event loop handling improvement
4. Add monitoring dashboard for shadow comparison metrics

---

📊 Overall Assessment

Code Quality: ⭐⭐⭐⭐ (4/5)
Architecture: ⭐⭐⭐⭐⭐ (5/5) - Shadow pattern is excellent
Test Coverage: ⭐⭐⭐ (3/5) - Good fixes, but incomplete
Security: ⭐⭐⭐ (3/5) - Minor information leakage issues
Performance: ⭐⭐⭐⭐ (4/5) - Well-designed, minor concerns

Recommendation: Approve with changes - Address HIGH priority items before merge. This is solid foundational work for cache consolidation.

---

Great work on this complex refactoring! The shadow harness pattern demonstrates strong architectural thinking. 🎉