Merge NodeOperations into NodeService to eliminate architectural duplication

[malibio]

Merge NodeOperations into NodeService to eliminate architectural duplication

Overview

NodeSpace currently has two layers performing duplicate business logic, creating confusion and dangerous bypass opportunities. This issue merges NodeOperations into NodeService to create a single, unified business logic layer.

Problem Statement

Current Architecture (Problematic)

Entry Points (Tauri/HTTP/MCP)
    ↓
NodeOperations (2,504 lines)
    ↓  
NodeService (4,473 lines)
    ↓
SurrealStore (Database)

Critical Issues

1. Duplicate Validation: Both layers validate parent existence, node existence, etc.
2. Split Critical Logic: Circular reference check only exists in NodeService (not NodeOperations!)
3. Dangerous Bypasses: Tauri move_node command bypasses BOTH layers, calling SurrealStore directly
4. Unclear Responsibilities: Both claim to handle "business logic"

The Dangerous Bypass

Current Tauri move_node (packages/desktop-app/src-tauri/src/commands/nodes.rs:413):

#[tauri::command]
pub async fn move_node(
    store: State<'_, SurrealStore>,  // ❌ Bypasses ALL validation!
    node_id: String,
    new_parent_id: Option<String>,
    insert_after_node_id: Option<String>,
) -> Result<(), CommandError> {
    store.move_node(&node_id, new_parent_id.as_deref(), insert_after_node_id.as_deref())
        .await
        .map_err(Into::into)
}

What gets bypassed:

• ❌ Root node validation (roots can't be moved)
• ❌ Circular reference check (can create infinite loops!)
• ❌ Parent existence validation
• ❌ Version conflict checking (OCC)
• ❌ Domain event emission

Self-Contained Scope

• Single unified business logic layer (NodeService)
• All entry points call NodeService (no bypasses possible)
• Comprehensive validation in one place
• Event emission centralized
• No breaking changes to external APIs
• Full test coverage maintained

Implementation Approach: Phased Delivery

Phase 1: Audit Current State

Goal: Document what's in each layer and identify overlaps

Tasks:

• List all NodeOperations methods
• List all NodeService methods
• Identify duplicate validations
• Map entry point call patterns (Tauri/HTTP/MCP)
• Create overlap matrix

Deliverable: Audit document showing which validations are duplicated

Phase 2: Merge move_node (Proof of Concept)

Goal: Prove the merge pattern works with the most critical method

Tasks:

• Update NodeService::move_node to include ALL validations from NodeOperations
• Add OCC version checking
• Add root node validation
• Ensure circular reference check is present
• Add sibling positioning support
• Update Tauri command to call NodeService (not SurrealStore)
• Write test for circular reference prevention

Key Implementation:

// NodeService::move_node (unified version)
pub async fn move_node(
    &self,
    node_id: &str,
    expected_version: i64,  // ADD: OCC support
    new_parent: Option<&str>,
    insert_after: Option<&str>,  // ADD: Sibling positioning
) -> Result<(), NodeServiceError> {
    // 1. Verify node exists + check version
    let node = self.get_node(node_id).await?
        .ok_or_else(|| NodeServiceError::node_not_found(node_id))?;
    
    if node.version != expected_version {
        return Err(NodeServiceError::version_conflict(/* ... */));
    }
    
    // 2. Check if root node (can't move roots)
    if self.is_root_node(node_id).await? {
        return Err(NodeServiceError::invalid_operation(
            "Root nodes cannot be moved"
        ));
    }
    
    // 3. Validate new parent + circular reference check
    if let Some(parent_id) = new_parent {
        self.get_node(parent_id).await?
            .ok_or_else(|| NodeServiceError::invalid_parent(parent_id))?;
        
        // CRITICAL: Prevent infinite loops
        if self.is_descendant(node_id, parent_id).await? {
            return Err(NodeServiceError::circular_reference(/* ... */));
        }
    }
    
    // 4. Perform move
    self.store.move_node(node_id, new_parent, insert_after).await?;
    
    // 5. Emit event
    self.emit_event(NodeMoved { node_id, new_parent }).await;
    
    Ok(())
}

Acceptance Criteria:

• NodeService::move_node includes all validations
• Circular reference test passes
• Tauri command calls NodeService (not SurrealStore)
• All existing move_node tests pass

Phase 3: Merge create_node

Goal: Migrate node creation with date container auto-creation

Tasks:

• Copy helper methods from NodeOperations to NodeService:
  • ensure_date_container_exists()
  • resolve_container()
  • calculate_sibling_position()
  • validate_parent_hierarchy()
• Update NodeService::create_node to use these helpers
• Update all entry points (Tauri, HTTP, MCP)
• Verify date nodes auto-create when referenced

Acceptance Criteria:

• Date containers auto-create when needed
• Container/root auto-derived from parent chain
• Sibling positioning works correctly
• All create_node tests pass
• Entry points updated

Phase 4: Merge update_node and delete_node

Goal: Complete core CRUD operations

Tasks:

• Merge version conflict handling into NodeService::update_node
• Merge cascade deletion into NodeService::delete_node
• Ensure update restrictions enforced (content vs hierarchy)
• Update entry points
• Verify event emission

Acceptance Criteria:

• Update operations check versions properly
• Delete operations cascade correctly
• Hierarchy changes use separate methods
• All CRUD tests pass

Phase 5: Update All Entry Points

Goal: Ensure ALL entry points call NodeService (prevent bypasses)

Files to Update:

1. Tauri Commands (packages/desktop-app/src-tauri/src/commands/nodes.rs):

   • Update all commands to use State<Arc<NodeService>>
   • Remove State<Arc<SurrealStore>> from signatures

2. HTTP Endpoints (packages/desktop-app/src-tauri/src/bin/dev-proxy.rs):

   • Verify all use State<Arc<NodeService>>

3. MCP Handlers (if applicable):

   • Verify NodeService usage

4. Tauri State Setup:

// BEFORE (allows bypasses):
app.manage(Arc::new(store.clone()));  // ❌ Exposed to commands

// AFTER (prevents bypasses):
app.manage(Arc::new(node_service.clone()));  // ✅ Only service exposed

Acceptance Criteria:

• All Tauri commands use State<Arc<NodeService>>
• No commands use State<Arc<SurrealStore>>
• No commands use State<Arc<NodeOperations>>
• Bypasses are impossible (compiler enforced)

Phase 6: Delete NodeOperations Module

Goal: Remove duplicate layer completely

Tasks:

• Delete packages/core/src/operations/ directory
• Remove from packages/core/src/lib.rs
• Update imports throughout codebase
• Verify compilation

Commands:

git rm -rf packages/core/src/operations/
# Update lib.rs to remove: pub mod operations;
# Find and update imports: rg "use.*operations::" packages/

Acceptance Criteria:

• operations/ module deleted
• No compilation errors
• All imports updated
• Tests still pass

Phase 7: Testing and Validation

Goal: Verify no regressions

Test Strategy:

# Full Rust test suite
cargo test --package nodespace-core

# Frontend tests
bun run test:all

# Specific scenarios
cargo test circular_reference
cargo test move_node
cargo test hierarchy

New Tests to Add:

#[tokio::test]
async fn test_move_node_prevents_circular_reference() {
    // Create: A -> B -> C
    // Try: Move A under C (should fail)
    // Assert: CircularReference error
}

#[tokio::test]
async fn test_root_nodes_cannot_be_moved() {
    // Create date node (root)
    // Try to move it
    // Assert: InvalidOperation error
}

Acceptance Criteria:

• All existing tests pass (100% pass rate)
• New circular reference test added and passes
• New root node test added and passes
• Manual testing confirms no regressions

Phase 8: Documentation and Safeguards

Goal: Prevent this architectural duplication from happening again

Documentation Updates:

1. CLAUDE.md - Add architecture rules:

## Business Logic Architecture Rules

**SINGLE LAYER ONLY:**
- ✅ All business logic lives in `NodeService`
- ❌ NO separate "operations" or "use cases" layer
- ❌ NO bypassing NodeService to call SurrealStore

**Entry Point Pattern:**
```rust
// ✅ CORRECT
#[tauri::command]
async fn do_thing(service: State<Arc<NodeService>>) {
    service.do_thing().await
}

// ❌ WRONG - Bypasses validation
#[tauri::command]
async fn do_thing(store: State<Arc<SurrealStore>>) {
    store.do_thing().await
}

2. **NodeService Rustdoc** - Add critical warnings:

```rust
//! # NodeSpace Business Logic Layer
//!
//! ⚠️ **CRITICAL:** ALL entry points MUST call NodeService
//! NEVER call SurrealStore directly - bypasses validation!
//!
//! Missing validations when bypassing:
//! - Circular reference prevention
//! - Version conflict detection  
//! - Root node constraints
//! - Domain event emission

3. Architecture Docs - Update docs/architecture/business-logic/overview.md

Acceptance Criteria:

• CLAUDE.md updated with clear rules
• NodeService has comprehensive rustdoc warnings
• Architecture docs reflect single-layer design
• Code review checklist created

Acceptance Criteria

Must Have

• All phases (1-8) completed
• NodeOperations module deleted
• All entry points call NodeService (no bypasses possible)
• All tests pass (no regressions)
• Circular reference check working in move_node
• Documentation updated to prevent future duplication
• Code review completed

Should Have

• Performance validated (no significant regression)
• Manual testing performed
• Line count reduction documented

Nice to Have

• Before/after architecture diagrams
• Migration lessons documented

Technical Specifications

Reference Files

Current NodeOperations:

• packages/core/src/operations/mod.rs - 2,504 lines
• packages/core/src/operations/error.rs - Error types
• packages/core/src/operations/sibling_queue.rs - Sibling ordering

Target NodeService:

• packages/core/src/services/node_service.rs - 4,473 lines (will absorb operations logic)

Entry Points to Update:

• packages/desktop-app/src-tauri/src/commands/nodes.rs - Tauri commands
• packages/desktop-app/src-tauri/src/bin/dev-proxy.rs - HTTP endpoints
• packages/core/src/mcp/ - MCP handlers (if exist)

System Requirements

Testing:

• Rust test suite: cargo test --package nodespace-core
• Frontend tests: bun run test:all
• Manual testing checklist provided

Tools:

• cargo clippy - Code quality checks
• rg (ripgrep) - Find usages
• cloc - Count lines of code

Resources & References

Architecture Documentation

• Senior architect review: [Link to conversation/analysis]
• docs/architecture/business-logic/overview.md - Current (outdated) architecture
• docs/architecture/data/surrealdb-schema-design.md - Database layer

Related Code

• packages/core/src/services/node_service.rs - Target for merged logic
• packages/core/src/db/surreal_store.rs - Database layer (should not be called directly)

Testing Utilities

• packages/core/tests/ - Integration tests
• Test helpers in node_service.rs

Definition of Done

• All phases completed successfully
• Zero compilation errors
• All tests pass (100% pass rate)
• No entry points bypass NodeService
• Circular reference prevention working
• Code review completed
• Documentation updated
• Ready to merge

Related Issues

• Create NodeOperations Business Logic Layer #332 (original NodeOperations creation - incomplete refactoring)

---

Note for implementers: This is a refactoring task suitable for junior engineers with mentorship. Each phase builds on the previous, with clear acceptance criteria. The compiler will help catch most issues. When stuck, ask for help in issue comments with specific error messages.

[malibio]

Pre-Implementation Checklist for Implementers

Before starting any work on this issue, complete the mandatory startup sequence:

Mandatory Startup Sequence

# 1. Check git status and commit any pending changes
git status

# 2. Pull latest changes
git fetch origin && git pull origin main

# 3. Run test baseline (BOTH frontend AND backend)
bun run test & cargo test --package nodespace-core & wait

# 4. Document baseline in this issue
# Comment with: "Test baseline: X frontend pass/Y fail, Z backend pass/W fail"

# 5. Create feature branch
git checkout -b feature/issue-676-merge-node-operations

# 6. Assign issue to yourself
bun run gh:assign 676 "@me"

# 7. Update project status
bun run gh:status 676 "In Progress"

Architecture Context

This refactoring is based on a comprehensive senior architect review that identified:

• Duplicate validation across NodeOperations and NodeService
• Critical logic split (circular reference check only in NodeService)
• Dangerous bypasses (Tauri move_node calling SurrealStore directly)

Key principle: After this refactoring, there should be ONE place for business logic (NodeService) and ONE place entry points call (also NodeService).

Getting Started

1. Complete the startup sequence above
2. Start with Phase 1 (Audit) - understand what's in each layer
3. Use the proof-of-concept approach (Phase 2 with move_node)
4. Let the compiler guide you - it will catch most issues
5. Ask questions in comments if you get stuck

Reference Documentation

• CLAUDE.md - Development process and standards
• docs/architecture/development/process/startup-sequence.md - Mandatory pre-implementation steps
• docs/architecture/business-logic/overview.md - Current architecture (will be updated)

[malibio]

Test Baseline

Frontend Tests: 1493 passed (75 test files) ✅
Backend Tests: 139 passed (14 ignored) ✅

Starting implementation on branch feature/issue-676-merge-node-operations