Codebase MCP Server

# Review Summary: Bug #015 Project Resolution Fixes **Date**: 2025-10-19 **Reviewers**: Code Reviewer, Debugger Specialist, Software Architect **Status**: APPROVED WITH CHANGES ## Executive Summary Three specialized subagents reviewed the bug fix proposal. **All reviewers confirmed the 5 bugs are real and correctly diagnosed**, with the database name `cb_proj_workflow_mcp_0d1eea82` serving as conclusive proof of workflow-mcp fallthrough contamination. **Key Findings**: - ✅ **2 fixes approved as-is** (Fix 3, Fix 5) - ⚠️ **1 fix approved with changes** (Fix 2) - ❌ **2 fixes rejected** (Fix 1, Fix 4) - need redesign - 🚨 **6 additional critical bugs discovered** - 🏗️ **Root cause is architectural complexity** (4-tier resolution chain) ## Review Results ### Code Reviewer Assessment **Overall Quality**: 4.5/10 (needs significant rework) | Fix | Status | Primary Issues | |-----|--------|----------------| | **Fix 1** | ❌ REJECT | Logical contradiction: calls Tier 2 logic when explicit ID provided | | **Fix 2** | ⚠️ APPROVE_WITH_CHANGES | Exception handling breaks Tier 3 fallback entirely | | **Fix 3** | ✅ APPROVE | Sound validation, minor timing improvements suggested | | **Fix 4** | ❌ REJECT | Violates encapsulation, incomplete (PostgreSQL not fixed) | | **Fix 5** | ✅ APPROVE | Pure additive logging, helps debugging | **Critical Code Issues**: 1. **Fix 1**: When explicit ID provided, should create project directly or fail - NOT delegate to session config 2. **Fix 2**: `raise` statement outside `if config_path:` block breaks all fallback scenarios 3. **Fix 4**: Direct access to `_projects_by_id` violates OOP principles, needs `ProjectRegistry.remove()` method ### Debugger Specialist Assessment **Root Cause Validation**: ✅ All 5 bugs CONFIRMED **Additional Bugs Discovered**: 6 new critical issues | Bug # | Severity | Description | |-------|----------|-------------| | **Bug 6** | MEDIUM | Race condition in PostgreSQL registry sync (ON CONFLICT DO UPDATE) | | **Bug 7** | HIGH | Database name override ignored in registry lookup | | **Bug 8** | MEDIUM | No validation of tier resolution order (silent fallthrough) | | **Bug 9** | HIGH | Circular dependency: config fails → returns None → falls through to wrong project | | **Bug 10** | LOW | Cache invalidation only checks mtime, not content hash | | **Bug 11** | LOW | No protection against infinite recursion in proposed fixes | **Database Name Evidence**: ✅ CONCLUSIVE - Name `cb_proj_workflow_mcp_0d1eea82` definitively proves project came from workflow-mcp - Sanitized name "workflow_mcp" could ONLY come from project named "workflow-mcp" - User expected "cb_proj_commission_processing_ve_*" **Trace Validation**: Confirmed exact flow: 1. Tier 1: explicit_id not found → falls through ❌ 2. Tier 2: config resolution fails → falls through ❌ 3. Tier 3: workflow-mcp returns "workflow-mcp" project ✓ 4. Result: Wrong database `cb_proj_workflow_mcp_0d1eea82` ❌ ### Software Architect Assessment **Recommendation**: Proceed with fixes, but **deprecate Tier 3 in v3.0** **Constitutional Violations**: - ❌ **Principle I (Simplicity)**: 4-tier resolution + dual-registry = unnecessary complexity - ❌ **Principle II (Local-First)**: Tier 3 requires external HTTP service - ❌ **Principle V (Production Quality)**: Silent failures, unclear errors **Architectural Root Cause**: > "4 of 5 bugs stem from **architectural complexity**, not just implementation errors. The 4-tier fallthrough creates 'fallthrough hell' where explicit user intent is overridden by automatic fallbacks." **Key Architectural Issues**: 1. **Tier 3 violates local-first**: workflow-mcp project ≠ codebase-mcp project (different domains) 2. **Dual-registry fragility**: In-memory + PostgreSQL must stay synchronized, currently no cascade invalidation 3. **Fallthrough overrides intent**: User says "use Project A", system gives "Project B" from workflow-mcp ## Consensus Recommendations ### Immediate Actions (v2.0.1 - Next Commit) **APPROVE** (implement immediately): 1. ✅ **Fix 3**: Database validation after provisioning 2. ✅ **Fix 5**: Diagnostic logging at each tier **REDESIGN** (before implementing): 3. ⚠️ **Fix 2**: Prevent workflow-mcp fallthrough - **Change**: Modify `_resolve_project_context()` to raise exception when config exists but resolution fails - **Reason**: Simpler than checking config existence twice 4. ❌ **Fix 1**: Tier 1 fallthrough prevention - **Change**: Create project directly with explicit ID, don't delegate to Tier 2 - **Reason**: Honors explicit ID contract, avoids circular logic 5. ❌ **Fix 4**: In-memory registry cleanup - **Change**: Add `ProjectRegistry.remove(project)` method, don't access private attributes - **Reason**: Proper encapsulation, maintainable ### Additional Fixes Required **Bug 7** (HIGH): Database name override not synced to registry ```python if database_name_override and database_name != row['database_name']: await conn.execute( "UPDATE projects SET database_name = $1 WHERE id = $2", database_name, row['id'] ) ``` **Bug 9** (HIGH): Circular dependency in project resolution ```python # In _resolve_project_context(), line 416: except Exception as e: if config_path: # Config exists but failed raise # Don't allow fallthrough return None # No config, graceful fallthrough ``` ### Testing Requirements **Must Pass Before Merge**: 1. Test Case 1: Config with new project ID (from analysis.md) 2. Test Case 2: Config ID change scenario (from analysis.md) 3. Test Case 3: No workflow-mcp fallthrough (from analysis.md) 4. **NEW**: Test Case 4: Registry unavailable (should raise error, not fall through) 5. **NEW**: Test Case 5: Database provisioning failure (should raise error with root cause) 6. **NEW**: Test Case 6: Simultaneous project creation (race condition) ### Medium-Term (v2.1 - Next Minor Release) **Deprecation Phase**: 1. Add deprecation warning when Tier 3 (workflow-mcp) is used 2. Document migration path: "Use `.codebase-mcp/config.json` instead" 3. Add project lifecycle tools: `list_projects()`, `update_project()`, `archive_project()` **Registry Consolidation**: 1. Move to PostgreSQL-only registry with async LRU cache 2. Eliminate in-memory registry synchronization issues 3. Add `cleanup_orphaned_databases()` utility ### Long-Term (v3.0 - Major Version) **Breaking Changes** (requires migration guide): 1. **Remove Tier 3 entirely** (workflow-mcp integration) 2. Simplify to 3-tier resolution: Explicit ID → Config → Default 3. PostgreSQL-only registry (single source of truth) **Constitutional Compliance Restored**: - ✅ Principle I: Simplicity (3 tiers vs 4) - ✅ Principle II: Local-first (no external HTTP) - ✅ Principle V: Production quality (clear errors) ## Implementation Priority ### Phase 1: Critical Fixes (This Branch) **Timeline**: 1-2 days ``` 1. Implement Fix 5 (logging) ← Start here 2. Implement Fix 3 (database validation) 3. Redesign & implement Fix 2 (prevent fallthrough) 4. Redesign & implement Fix 1 (Tier 1 auto-create) 5. Redesign & implement Fix 4 (registry cleanup) 6. Fix Bug 7 (database name override sync) 7. Fix Bug 9 (circular dependency) 8. Add all 6 test cases 9. Update error messages for clarity 10. Add project_resolution metadata to MCP responses ``` ### Phase 2: Deprecation Warning (v2.1) **Timeline**: 2-4 weeks ``` 1. Add deprecation log when Tier 3 used 2. Document migration guide 3. Registry consolidation (PostgreSQL-only) 4. Project lifecycle management tools ``` ### Phase 3: Simplification (v3.0) **Timeline**: 3-6 months ``` 1. Remove Tier 3 code entirely 2. Update all tests 3. Migration guide for users 4. Constitutional compliance verification ``` ## Risk Assessment ### Short-Term Fixes - **Risk**: LOW - **Reason**: Fixes are well-scoped, testable, incremental - **Mitigation**: Comprehensive test suite, integration tests ### Long-Term Simplification - **Risk**: MEDIUM - **Reason**: Breaking change (Tier 3 removal) - **Mitigation**: Phased deprecation (v2.1 warnings), migration guide, 6-month timeline ## Reviewer Consensus **All three reviewers agree**: 1. ✅ Bugs are real and correctly diagnosed 2. ✅ Database name evidence is conclusive proof 3. ⚠️ Proposed fixes need redesign before implementation 4. 🏗️ Root cause is architectural (4-tier complexity) 5. 📋 Long-term: Deprecate Tier 3, simplify to 3 tiers **Code Reviewer**: "DO NOT MERGE fixes as currently proposed. Redesign Fix 1, 2, 4 per recommendations." **Debugger**: "All 5 bugs confirmed. Found 6 additional critical bugs. Trace validates exact user scenario." **Architect**: "Approve short-term fixes with enhancements. Plan long-term architectural simplification. Tier 3 violates constitutional principles." ## Final Recommendation **Status**: ✅ APPROVED WITH CHANGES **Next Steps**: 1. Implement redesigned fixes per reviewer recommendations 2. Add 6 test cases (3 from analysis + 3 new) 3. Fix Bugs 7 and 9 (discovered by debugger) 4. Add project resolution metadata to all MCP responses 5. Merge to branch, create PR with comprehensive tests **Do NOT merge** original Fix 1, 2, 4 as proposed - implement redesigns instead.