Repository navigation
Prepare local runtimes through LocalDaemonExecutor - #439
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 37 minutes and 44 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR establishes a runtime preparation boundary for local-daemon workspaces by adding a ChangesLocal Daemon Executor Runtime Preparation Boundary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
coding-agent/src/coding_agent/server/session_manager.py (1)
4707-4722:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBuild from the validated preparation request, not
session.default_run_target.
LocalDaemonExecutor.prepare_runtime()validatesLocalDaemonRuntimePreparation.request, but this callback drops that request and_build_session_runtime_direct()re-readssession.default_run_target. That means the executor can validate one target and the provider can still prepare a different runtime if session state changes before execution. Please thread theRunRequest/RunTargetinto the direct builder and resolve the environment from that validated request instead of mutable session state.Suggested direction
- async def _build_session_runtime_direct( + async def _build_session_runtime_direct( self, session: Session, *, + target: RunTarget | None = None, model_name: str | None = None, provider_name: str | None = None, base_url: str | None = None, max_steps: int | None = None, approval_policy: ApprovalPolicy | None = None, ) -> tuple[Any, Any, PipelineAdapter]: @@ - environment = self._resolve_environment_for_run_target(session.default_run_target) + environment = self._resolve_environment_for_run_target( + session.default_run_target if target is None else target + ) @@ - async def prepare_runtime(_: RunRequest) -> LocalDaemonRuntimeBinding: + async def prepare_runtime(request: RunRequest) -> LocalDaemonRuntimeBinding: pipeline, ctx, adapter = await self._build_session_runtime_direct( session, + target=request.target, model_name=model_name, provider_name=provider_name, base_url=base_url, max_steps=max_steps, approval_policy=approval_policy,Also applies to: 4765-4786
🤖 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 `@coding-agent/src/coding_agent/server/session_manager.py` around lines 4707 - 4722, The code is using mutable session.default_run_target instead of the validated preparation request; update LocalDaemonExecutor.prepare_runtime to pass the validated RunRequest/RunTarget into _build_session_runtime_direct and change _build_session_runtime_direct to accept that RunRequest/RunTarget parameter; then use that passed-in run target when calling _resolve_environment_for_run_target and _environment_workspace_root (instead of session.default_run_target) and forward the same validated target/context into _create_agent_for_session (where workspace_root and environment are computed), and apply the same threading change for the duplicate block around the other occurrence (~4765-4786) so the prepared-and-validated request is always used instead of mutable session state.
🧹 Nitpick comments (3)
coding-agent/tests/ui/test_session_manager_runtime.py (3)
876-878: 💤 Low valueConsider more explicit assertions.
The list comprehension assertion is correct but could be clearer by separating the length check from the target comparison. This makes test failures more informative.
📊 Suggested alternative
- assert [preparation.request.target for preparation in local_executor.preparations] == [ - session.default_run_target - ] + assert len(local_executor.preparations) == 1 + assert local_executor.preparations[0].request.target == session.default_run_targetThis makes it immediately clear whether the failure is due to the wrong number of preparations or the wrong target.
🤖 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 `@coding-agent/tests/ui/test_session_manager_runtime.py` around lines 876 - 878, The test currently uses a single list-comprehension assertion that mixes count and value checks; change it to two explicit assertions after creating the session (session_id = await manager.create_session()): first assert the number of preparations (e.g., assert len(preparations) == expected_count) and then assert the specific target value (e.g., assert preparations[0].target == "expected_target"), referencing the preparations produced by the FakeAdapter to make failures clearer.
943-945: 💤 Low valueConsider more explicit assertions.
Same suggestion as lines 876-878: separate the length check from the target comparison for clearer test failure messages.
📊 Suggested alternative
- assert [preparation.request.target for preparation in local_executor.preparations] == [ - session.default_run_target - ] + assert len(local_executor.preparations) == 1 + assert local_executor.preparations[0].request.target == session.default_run_target🤖 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 `@coding-agent/tests/ui/test_session_manager_runtime.py` around lines 943 - 945, The test currently compares a collection to the expected AgentRunRecord in one step; change it to two explicit assertions so failures are clearer: first assert the length (e.g., assert len(runs) == 1) and then assert the contents match (e.g., compare runs[0] to previous_run or assert specific fields), referencing the existing AgentRunRecord instance named previous_run and the collection variable runs in test_session_manager_runtime.py to locate where to split the checks.
345-350: 💤 Low valueConsider adding return type annotation.
The
prepare_runtimemethod override is missing a return type annotation. While this is test code and not strictly required, adding the annotation would improve consistency with the base class interface and make the test double's contract clearer.📝 Suggested enhancement
async def prepare_runtime( self, preparation: LocalDaemonRuntimePreparation, -): +) -> LocalExecutionBinding: self.preparations.append(preparation) return await preparation.runtime_provider.prepare_runtime(preparation.request)🤖 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 `@coding-agent/tests/ui/test_session_manager_runtime.py` around lines 345 - 350, The test double method prepare_runtime should include an explicit return type to match the base class contract: add the appropriate coroutine return annotation (e.g., -> Awaitable[RuntimeType] or -> Coroutine[Any, Any, RuntimeType] or the concrete Runtime type used in the codebase) to the prepare_runtime signature; update the signature on the LocalDaemonRuntimePreparation-handling method prepare_runtime to include that return type and ensure imports (typing.Awaitable/Coroutine or the runtime type) are added if necessary so the override's type matches the runtime_provider.prepare_runtime return type.
🤖 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
`@coding-agent/.opencode/prompts/tasks/local-daemon-pr17-runtime-preparation-boundary.md`:
- Around line 47-50: The pytest invocation in the prompt uses a direct path to
tests/ui/test_session_manager_runtime.py which is outside the documented suites;
update the second command in local-daemon-pr17-runtime-preparation-boundary.md
to call the appropriate suite-level pytest entry (e.g., run pytest against the
tests/coding_agent/ or the repo's defined suite that includes ui tests) or add a
short justification comment explaining why that specific test file must be
invoked directly; change the line that currently references
tests/ui/test_session_manager_runtime.py to the suite-level invocation per the
repo guideline (tests/coding_agent/ or the agreed suite) or add the
justification text inline so reviewers understand the intentional exception.
In `@coding-agent/tests/coding_agent/test_local_daemon_executor.py`:
- Around line 355-396: Add a new async test that mirrors
test_local_daemon_executor_rejects_runtime_preparation_for_non_local_target but
flips the mismatch: construct a RunRequest whose target.executor is a
LocalDaemon executor ref (e.g., LocalDaemonExecutorRef or the local-daemon
binding used by LocalDaemonExecutor) while target.workspace is a non-local
CloudWorkspaceRef, then call
LocalDaemonExecutor().prepare_runtime(LocalDaemonRuntimePreparation(...)) and
assert it raises RunExecutorTargetError (use pytest.raises with an appropriate
match for the local-workspace requirement). Ensure you use the same
FakeRuntimeProvider and FakeRuntimeAdapter setup and assert provider.requests
and adapter.prompts remain empty; this will cover the _validate_request_target()
branch that rejects local-daemon targets paired with non-local workspaces.
---
Outside diff comments:
In `@coding-agent/src/coding_agent/server/session_manager.py`:
- Around line 4707-4722: The code is using mutable session.default_run_target
instead of the validated preparation request; update
LocalDaemonExecutor.prepare_runtime to pass the validated RunRequest/RunTarget
into _build_session_runtime_direct and change _build_session_runtime_direct to
accept that RunRequest/RunTarget parameter; then use that passed-in run target
when calling _resolve_environment_for_run_target and _environment_workspace_root
(instead of session.default_run_target) and forward the same validated
target/context into _create_agent_for_session (where workspace_root and
environment are computed), and apply the same threading change for the duplicate
block around the other occurrence (~4765-4786) so the prepared-and-validated
request is always used instead of mutable session state.
---
Nitpick comments:
In `@coding-agent/tests/ui/test_session_manager_runtime.py`:
- Around line 876-878: The test currently uses a single list-comprehension
assertion that mixes count and value checks; change it to two explicit
assertions after creating the session (session_id = await
manager.create_session()): first assert the number of preparations (e.g., assert
len(preparations) == expected_count) and then assert the specific target value
(e.g., assert preparations[0].target == "expected_target"), referencing the
preparations produced by the FakeAdapter to make failures clearer.
- Around line 943-945: The test currently compares a collection to the expected
AgentRunRecord in one step; change it to two explicit assertions so failures are
clearer: first assert the length (e.g., assert len(runs) == 1) and then assert
the contents match (e.g., compare runs[0] to previous_run or assert specific
fields), referencing the existing AgentRunRecord instance named previous_run and
the collection variable runs in test_session_manager_runtime.py to locate where
to split the checks.
- Around line 345-350: The test double method prepare_runtime should include an
explicit return type to match the base class contract: add the appropriate
coroutine return annotation (e.g., -> Awaitable[RuntimeType] or ->
Coroutine[Any, Any, RuntimeType] or the concrete Runtime type used in the
codebase) to the prepare_runtime signature; update the signature on the
LocalDaemonRuntimePreparation-handling method prepare_runtime to include that
return type and ensure imports (typing.Awaitable/Coroutine or the runtime type)
are added if necessary so the override's type matches the
runtime_provider.prepare_runtime return type.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b42751b-86db-46d5-8575-5ae9001677cc
📒 Files selected for processing (6)
coding-agent/.opencode/prompts/tasks/local-daemon-pr17-runtime-preparation-boundary.mdcoding-agent/src/coding_agent/executors/__init__.pycoding-agent/src/coding_agent/executors/local_daemon.pycoding-agent/src/coding_agent/server/session_manager.pycoding-agent/tests/coding_agent/test_local_daemon_executor.pycoding-agent/tests/ui/test_session_manager_runtime.py
delete-cloud
force-pushed
the
dogfood/local-runtime-preparation-executor
branch
3 times, most recently
from
June 2, 2026 18:19
9bc0c9e to
a04c734
Compare
delete-cloud
force-pushed
the
dogfood/local-runtime-preparation-executor
branch
from
June 2, 2026 18:24
a04c734 to
3a815d4
Compare
This was referenced Jun 2, 2026
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
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
ADR-0058 Direction
This moves local REPL/runtime rebuild setup one step away from SessionManager-owned runtime construction and toward executor-owned local runtime preparation. Turn lifecycle persistence hooks and checkpoint restore are intentionally left for later slices.
Tests
Summary by CodeRabbit