feat(#160): test(ipc): create MockRuntime for deterministic agent testing#195
Merged
Conversation
WalkthroughAdds ChangesMockRuntime testing utility
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sova/ipc/testing.py`:
- Around line 58-69: The issue is that when wait() is sleeping on
_duration_seconds and stop() is called, the sleep continues uninterrupted and
blocks until the full duration expires, even though _returncode has already been
set. To fix this, introduce a cancellation event (similar to the existing
_hang_release event) that can interrupt the sleep. In the wait() method, replace
the await asyncio.sleep(_duration_seconds) call with asyncio.wait() using both
the duration timeout and the new cancellation event, so the sleep can be
interrupted early. In the stop() method, set this cancellation event to wake up
any in-flight wait() call, allowing it to return immediately instead of blocking
on the remaining sleep duration.
In `@tests/test_ipc.py`:
- Around line 1333-1335: The stopper task created with
asyncio.create_task(stopper()) is not being tracked or awaited, which can hide
exceptions and leave background tasks in the event loop. Store the task returned
by asyncio.create_task(stopper()) in a variable before the proc.wait() call,
then await that task variable after the assertion to ensure it completes
properly and any exceptions are surfaced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f8dd9f94-6d0d-49c0-b13f-c1ab710e836c
⛔ Files ignored due to path filters (2)
.githooks/pre-pushis excluded by none and included by nonedocs/roadmap.htmlis excluded by!docs/**and included by none
📒 Files selected for processing (3)
sova/ipc/runtime.pysova/ipc/testing.pytests/test_ipc.py
xsovad06
commented
Jun 18, 2026
xsovad06
left a comment
xsovad06
left a comment
Owner
Author
There was a problem hiding this comment.
Code Review for PR #195
Critical bug: MockAgentProcess.stop() fails to interrupt wait() when duration_seconds > 0, causing stop() to appear non-functional for timed processes. The wait() continues sleeping for the full duration instead of returning immediately when stopped. This violates the spec and lacks test coverage. Additional issues: spawned_processes returns mutable internal state, and _finished event is dead code.
4 findings (4 inline, 0 in summary)
| Sev | Category | File | Finding |
|---|---|---|---|
| 8/10 | bug | sova/ipc/testing.py:58 |
MockAgentProcess.stop() does not interrupt wait() when duration_seconds > 0 and should_hang=False. If wait() is sleeping via asyncio.sleep(), calling stop() sets _returncode but doesn't interrupt the sleep. The wait() continues for the full duration_seconds instead of returning immediately, violating the spec requirement that stop() 'just unblocks wait()'. |
| 6/10 | testing | tests/test_ipc.py:1328 |
Missing test case for stop() interrupting wait() when duration_seconds > 0. The test suite covers should_hang=True (test_hang_and_stop) but not the scenario where a process with duration_seconds > 0 is stopped mid-sleep. This gap masks the bug where stop() doesn't actually interrupt the sleep. |
| 4/10 | design | sova/ipc/testing.py:150 |
spawned_processes property returns the internal _spawned list directly, allowing external code to mutate the runtime's state (e.g. test could call rt.spawned_processes.clear()). Best practice for collection properties is to return a copy. |
| 2/10 | design | sova/ipc/testing.py:43 |
_finished asyncio.Event is created and set in wait() and stop() but never awaited anywhere. This is dead code with no observable effect. |
Assessment: BLOCK -- critical issues must be fixed before merge
xsovad06
dismissed
coderabbitai[bot]’s stale review
Findings addressed in latest push.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sova/ipc/testing.py`:
- Around line 75-80: The `timeout` parameter in the `MockAgentProcess.stop()`
method is unused and flagged by Ruff with ARG002. To fix this while maintaining
interface compatibility, prefix the parameter name with an underscore (rename
`timeout` to `_timeout`) to explicitly signal to Ruff that this parameter is
intentionally unused.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 14fffd32-c85a-4075-bac0-8d1c93acf7d5
📒 Files selected for processing (2)
sova/ipc/testing.pytests/test_ipc.py
xsovad06
dismissed
coderabbitai[bot]’s stale review
Findings addressed in latest push.
The pre-push hook ran 5 checks (ShellCheck, Ruff, format, commands, permissions) but CI runs 7 additional invariant scripts. This gap allowed a commit-format violation to pass locally but fail in CI. Add all invariant scripts as check 6/6 in the pre-push hook.
Replace --> with ~~> in HTML comments to prevent Mermaid from interpreting instruction examples as actual graph edges.
Closes #160
Closes #160
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sova/ipc/testing.py`:
- Around line 126-144: In the spawn method of the MockAgentRuntime class, prefix
the unused parameters cwd, env, model, and max_budget_usd with an underscore
(e.g., _cwd, _env, _model, _max_budget_usd) to signal to Ruff that these
parameters are intentionally unused while maintaining interface compatibility
with the AgentRuntime.spawn() abstract method signature. This suppresses the
ARG002 violations without changing the method's functionality.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 77c123db-9d2b-434e-8f7d-13f8b5c1ca8c
⛔ Files ignored due to path filters (2)
.githooks/pre-pushis excluded by none and included by nonedocs/roadmap.htmlis excluded by!docs/**and included by none
📒 Files selected for processing (3)
sova/ipc/runtime.pysova/ipc/testing.pytests/test_ipc.py
Comment on lines
+126
to
+144
| async def spawn( | ||
| self, | ||
| prompt: str, | ||
| cwd: str | Path, | ||
| *, | ||
| env: dict[str, str] | None = None, | ||
| model: str | None = None, | ||
| max_budget_usd: Decimal | None = None, | ||
| ) -> MockAgentProcess: | ||
| self._last_prompt = prompt | ||
| process = MockAgentProcess( | ||
| stdout_lines_data=list(self._stdout_lines) if self._stdout_lines else None, | ||
| stderr_lines_data=list(self._stderr_lines) if self._stderr_lines else None, | ||
| exit_code=self._exit_code, | ||
| duration_seconds=self._duration_seconds, | ||
| should_hang=self._should_hang, | ||
| ) | ||
| self._spawned.append(process) | ||
| return process |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Unused parameters flagged by Ruff but required for interface compatibility.
The parameters cwd, env, model, and max_budget_usd are unused in this mock implementation but required by the AgentRuntime.spawn() abstract method signature. Ruff flags these as ARG002 violations. Prefix them with underscore to signal they're intentionally unused while maintaining interface compliance.
♻️ Proposed fix
async def spawn(
self,
prompt: str,
- cwd: str | Path,
+ _cwd: str | Path,
*,
- env: dict[str, str] | None = None,
- model: str | None = None,
- max_budget_usd: Decimal | None = None,
+ _env: dict[str, str] | None = None,
+ _model: str | None = None,
+ _max_budget_usd: Decimal | None = None,
) -> MockAgentProcess:
self._last_prompt = prompt🧰 Tools
🪛 Ruff (0.15.17)
[warning] 129-129: Unused method argument: cwd
(ARG002)
[warning] 131-131: Unused method argument: env
(ARG002)
[warning] 132-132: Unused method argument: model
(ARG002)
[warning] 133-133: Unused method argument: max_budget_usd
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sova/ipc/testing.py` around lines 126 - 144, In the spawn method of the
MockAgentRuntime class, prefix the unused parameters cwd, env, model, and
max_budget_usd with an underscore (e.g., _cwd, _env, _model, _max_budget_usd) to
signal to Ruff that these parameters are intentionally unused while maintaining
interface compatibility with the AgentRuntime.spawn() abstract method signature.
This suppresses the ARG002 violations without changing the method's
functionality.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MockRuntimeimplementation of theAgentRuntimeABC to enable deterministic, subprocess-free testing of agent lifecycle managementMagicMock+_empty_async_iterboilerplate scattered across dashboard and IPC testsMockAgentProcessinterface that duck-typesAgentProcessChanges
New Test Infrastructure (
sova/ipc/testing.py)MockAgentProcess: AgentProcess-compatible mock with configurable exit codes, output streams, duration simulation, and hang scenarios usingasyncio.EventMockRuntime: FullAgentRuntimeABC implementation returningMockAgentProcessinstances, with spawn tracking (spawned_processes) and prompt capture (last_prompt) for test assertionsAgentProcessinterface:pid,is_running,returncode,wait(),stop(),stdout_lines(),stderr_lines(),classify_exit(),wait_classified()Runtime Factory Registration (
sova/ipc/runtime.py)"mock"in_RUNTIMESdict with lazy import to avoid test-only code in production pathscreate_runtime()to handle callable factory functions for lazy initializationTest Coverage (
tests/test_ipc.py)TestMockAgentProcess: 6 scenarios covering immediate success, delayed completion, crash exit codes, hung process + stop, stdout/stderr streaming, staticclassify_exit()TestMockRuntime: 5 scenarios covering factory creation, spawn tracking, prompt capture,parse_output(),check_available()Review Guidance
MockAgentProcessdoes NOT subclassAgentProcessbecause the real class requires anasyncio.subprocess.Processin__init__. Verify the public interface match is complete (all 10 members)._RUNTIMES["mock"]uses a lambda wrapper to defer import oftesting.pyuntilcreate_runtime("mock")is called -- avoids test-only imports in production.should_hang=True+stop()interaction --wait()must block on anasyncio.Eventthatstop()releases immediately, with no real signal handling or deadlock risk.stdout_lines()/stderr_lines()async iterators return immediately when data lists are empty or None, matching realAgentProcessbehavior.Test Plan
pytest tests/test_ipc.py::TestMockAgentProcess tests/test_ipc.py::TestMockRuntime(13 test methods)create_runtime("mock")returnsMockRuntimeinstancemake lintcleanmake check(1652+ tests)asyncio.wait_for(process.wait(), timeout=0.5)raisesTimeoutErrorwhenshould_hang=True, completes afterstop()calledCloses #160