refactor: remove workflow context/session parameters

[serialx]

Summary

• pass runner instances into workflow logic and remove the shared Vibecore.runner state to eliminate race conditions
• update CLI, examples, and documentation to use runner.user_input, runner.print, and runner.run_agent
• add a migration guide and regression tests covering concurrent runs and the removed Vibecore.run_agent
• simplify workflow signatures by dropping explicit context/session parameters and relying on runner.context and runner.session

Testing

• uv run pytest
• uv run pyright
• uv run ruff check .

---

https://chatgpt.com/codex/tasks/task_e_68ede2705dec832ab1e100330208f546

[claude]

Claude finished @serialx's task —— View job

---

Code Review Complete ✅

Review Tasks

• Analyze the refactoring approach and architecture changes
• Review code quality and best practices
• Check for potential bugs or race conditions
• Assess performance implications
• Review security concerns
• Evaluate test coverage
• Review migration guide completeness

---

Overall Assessment

This is a well-executed refactoring that successfully addresses race conditions in the Flow Mode by removing shared state and passing runner instances directly to workflows. The changes improve thread safety and make the API more explicit.

✅ Strengths

Architecture Improvements

• Eliminates race conditions: Removing the shared Vibecore.runner state and passing runner instances directly prevents concurrent execution issues
• Cleaner separation of concerns: Each runner type (Textual, CLI, Static) is now self-contained with its own state
• More explicit dependencies: Workflow signatures now clearly show they depend on a runner
• Simplified API: No need to pass context/session explicitly since they're available via runner.context and runner.session

Code Quality

• Clean, consistent refactoring across all affected files
• Type hints properly updated with generics
• Good use of descriptive error messages (e.g., line 305-307 in flow.py)
• Proper backwards compatibility handling with clear error for removed method

Documentation

• Comprehensive migration guide with clear before/after examples
• Updated README with new API patterns
• Clear explanation of why the change was made

⚠️ Areas for Improvement

1. Test Coverage (High Priority)

The test coverage is minimal with only 2 basic tests:

Missing test scenarios:

• Error handling when workflow logic is not defined
• Context and session propagation through runners
• Runner-specific behavior (Textual vs CLI vs Static)
• Multiple concurrent workflows with different contexts
• Handoff scenarios in multi-agent flows
• Edge cases like NoUserInputLeft exception handling

Recommendation: Add comprehensive tests for:

# Test context/session propagation
async def test_runner_context_session_propagation():
    # Verify context and session are correctly passed to workflow
    
# Test error conditions
async def test_workflow_not_defined_error():
    # Verify proper error when workflow decorator not used

# Test runner-specific behaviors
async def test_cli_runner_user_input():
    # Mock stdin and verify CLI runner behavior

# Test concurrent workflows with different contexts
async def test_concurrent_workflows_different_contexts():
    # Run multiple workflows with different contexts simultaneously

2. Breaking Change Management (Medium Priority)

While the RuntimeError for Vibecore.run_agent() is good, consider:

• Adding a deprecation period with warnings before full removal
• Including the migration path directly in the error message:

raise RuntimeError(
    "Vibecore.run_agent() is no longer available. "
    "Use the runner provided to the workflow logic instead.\n"
    "Example: await runner.run_agent(...)\n"
    "See docs/runner_migration.md for migration guide."
)

3. Type Safety Enhancement (Low Priority)

The current typing could be stricter:

# Current
DecoratedCallable: TypeAlias = Callable[
    [VibecoreRunnerBase[TContext, TWorkflowReturn]],
    Coroutine[Any, Any, TWorkflowReturn],
]

# Could be more specific about the coroutine
DecoratedCallable: TypeAlias = Callable[
    [VibecoreRunnerBase[TContext, TWorkflowReturn]],
    Coroutine[None, None, TWorkflowReturn],  # More specific
]

4. Documentation Completeness (Low Priority)

The migration guide is good but could benefit from:

• Example of migrating a multi-agent workflow
• Common pitfalls and troubleshooting section
• Performance implications (if any)

🔒 Security Review

No security concerns identified. The refactoring:

• Doesn't introduce new attack vectors
• Maintains existing security boundaries
• Properly isolates runner instances

⚡ Performance Considerations

The changes should have neutral to positive performance impact:

• ✅ Eliminates potential lock contention from shared state
• ✅ Each runner maintains its own session/context without synchronization
• ✅ No additional allocations or overhead introduced

🐛 Potential Issues

1. Static Runner Input Handling

In VibecoreStaticRunner.user_input() (line 125-128):

async def user_input(self, prompt: str = "") -> str:
    if not self.inputs:
        raise NoUserInputLeft()
    return self.inputs.pop()  # This pops from the end

The pop() method removes from the end of the list, but inputs are extended at the end (line 140). This means inputs are consumed in LIFO order (last-in-first-out) rather than FIFO. Consider using pop(0) for FIFO behavior:

return self.inputs.pop(0)  # Pop from beginning for FIFO

2. Missing Validation

No validation that workflow_logic accepts the correct runner type. Could add runtime validation in the decorator.

📋 Recommendations

1. Priority 1: Expand test coverage significantly before merging
2. Priority 2: Enhance error messages for breaking changes
3. Priority 3: Fix the Static Runner input order issue if FIFO is intended
4. Consider: Adding integration tests with real agent interactions
5. Consider: Adding performance benchmarks to validate the concurrency improvements

Summary

This is a solid refactoring that achieves its goals of eliminating race conditions and simplifying the API. The code quality is high and the migration guide is helpful. The main concern is the limited test coverage which should be expanded before merging to ensure the refactoring doesn't introduce regressions.

Verdict: Approve with suggestions - primarily needs more comprehensive test coverage.

---