feat: Add conversation variable persistence layer

[tomerqodo]

Benchmark PR from qodo-benchmark#429

[greptile-apps]

Greptile Summary

Refactored conversation variable persistence to use a dedicated graph engine layer instead of direct database updates in variable assigner nodes, improving separation of concerns and architectural cleanliness.

Key changes:

• Extracted ConversationVariableUpdaterImpl from core.workflow to new services.conversation_variable_updater module
• Created ConversationVariablePersistenceLayer that listens to NodeRunSucceededEvent and persists conversation variable updates
• Refactored VariableAssignerNode (v1 and v2) to store updated variables in process_data instead of directly updating database
• Updated ReadOnlyVariablePool protocol to use selector sequence parameter instead of separate node_id/key parameters
• Added comprehensive test coverage for the new persistence layer

Issues found:

• Critical: ConversationVariableUpdaterImpl.update() creates SQLAlchemy session without context manager (line 18), violating style guide rule feat: dark theme icon support  #6 and causing resource leaks. The original implementation correctly used with Session(db.engine) as session:

Confidence Score: 2/5

• Critical resource leak bug in database session management must be fixed before merge
• Score reflects one critical bug that will cause database connection leaks in production. The architectural refactoring is sound and well-tested, but the session management regression from the original implementation makes this unsafe to merge as-is
• Pay close attention to api/services/conversation_variable_updater.py which has a critical session management bug

Important Files Changed

Filename	Overview
api/services/conversation_variable_updater.py	New service file with critical session management bug - missing context manager
api/core/app/layers/conversation_variable_persist_layer.py	New persistence layer for conversation variables, handles NodeRunSucceededEvent filtering and updates
api/core/app/apps/advanced_chat/app_runner.py	Added ConversationVariablePersistenceLayer to the graph engine layers
api/core/workflow/nodes/variable_assigner/v1/node.py	Refactored to use process_data with common_helpers instead of direct database updates
api/core/workflow/nodes/variable_assigner/v2/node.py	Refactored to use process_data with common_helpers instead of direct database updates

Sequence Diagram

sequenceDiagram
    participant User
    participant AppRunner as AdvancedChatAppRunner
    participant WorkflowEntry
    participant GraphEngine
    participant PersistLayer as ConversationVariablePersistenceLayer
    participant VariableAssigner as VariableAssignerNode (v1/v2)
    participant Updater as ConversationVariableUpdaterImpl
    participant DB as Database

    User->>AppRunner: run()
    AppRunner->>WorkflowEntry: create WorkflowEntry
    AppRunner->>Updater: conversation_variable_updater_factory()
    Updater-->>AppRunner: updater instance
    AppRunner->>PersistLayer: new ConversationVariablePersistenceLayer(updater)
    AppRunner->>GraphEngine: layer(persistence_layer)
    
    GraphEngine->>VariableAssigner: execute node
    VariableAssigner->>VariableAssigner: _run()
    VariableAssigner->>VariableAssigner: update variable in variable_pool
    VariableAssigner->>VariableAssigner: set updated_variables in process_data
    VariableAssigner-->>GraphEngine: NodeRunSucceededEvent with process_data
    
    GraphEngine->>PersistLayer: on_event(NodeRunSucceededEvent)
    PersistLayer->>PersistLayer: check if VARIABLE_ASSIGNER node
    PersistLayer->>PersistLayer: extract updated_variables from process_data
    PersistLayer->>PersistLayer: filter for CONVERSATION_VARIABLE_NODE_ID
    
    loop For each conversation variable
        PersistLayer->>PersistLayer: get variable from variable_pool
        PersistLayer->>Updater: update(conversation_id, variable)
        Updater->>DB: SELECT ConversationVariable
        Updater->>DB: UPDATE data field
        Updater->>DB: commit()
        PersistLayer->>Updater: flush()
    end
    
    PersistLayer-->>GraphEngine: continue execution

Loading

[greptile-apps]

_{14 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

syntax: session created without context manager, violates style guide rule #6 and causes resource leak

Suggested change

	session = Session(db.engine)
	row = session.scalar(stmt)
	if not row:
	raise ConversationVariableNotFoundError("conversation variable not found in the database")
	row.data = variable.model_dump_json()
	session.commit()
	with Session(db.engine, expire_on_commit=False) as session:
	row = session.scalar(stmt)
	if not row:
	raise ConversationVariableNotFoundError("conversation variable not found in the database")
	row.data = variable.model_dump_json()
	session.commit()

Context Used: Context from dashboard - CLAUDE.md (source)

Prompt To Fix With AI

This is a comment left during a code review.
Path: api/services/conversation_variable_updater.py
Line: 18:23

Comment:
**syntax:** session created without context manager, violates style guide rule #6 and causes resource leak

```suggestion
        with Session(db.engine, expire_on_commit=False) as session:
            row = session.scalar(stmt)
            if not row:
                raise ConversationVariableNotFoundError("conversation variable not found in the database")
            row.data = variable.model_dump_json()
            session.commit()
```

**Context Used:** Context from `dashboard` - CLAUDE.md ([source](https://app.greptile.com/review/custom-context?memory=a5f96311-5bd1-49f2-9828-2ee0c089c012))

How can I resolve this? If you propose a fix, please make it concise.