Repository navigation
feat: split session record runtime handle - #423
Conversation
|
Warning Review limit reached
More reviews will be available in 51 minutes and 26 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 (3)
📝 WalkthroughWalkthroughThis PR implements ADR-0058 by refactoring Session to separate durable metadata (SessionRecord) from process-local runtime state (SessionRuntimeHandle). Session now delegates runtime field access to the handle via magic methods, and persistence is updated to exclude runtime state while preserving backward compatibility. ChangesSession State Separation for ADR-0058
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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}
There was a problem hiding this comment.
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)
857-889:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
approval_coordinatorin sync whenapproval_storechanges.After this split,
approval_storeis still a public mutable field, butapproval_coordinatornow lives onruntime_handle._hydrate_session()has to repair both manually, and any othersession.approval_store = ...assignment will silently leave the coordinator bound to the old store. That breaks approval reads/writes against the replacement store while preserving the old public API shape.Suggested fix
def __setattr__(self, name: str, value: object) -> None: + if name == "approval_store": + object.__setattr__(self, name, value) + instance_dict = object.__getattribute__(self, "__dict__") + handle = instance_dict.get("runtime_handle") + if handle is not None: + handle.approval_coordinator = ApprovalCoordinator( + cast(ApprovalStore, value) + ) + return if name in self._RUNTIME_HANDLE_FIELD_NAMES: instance_dict = object.__getattribute__(self, "__dict__") handle = instance_dict.get("runtime_handle") if handle is not None: setattr(handle, name, value) return object.__setattr__(self, name, value)Also applies to: 915-922
🤖 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 857 - 889, Make sure the ApprovalCoordinator is kept in sync whenever the public approval_store is replaced: change approval_store into a property with a setter (or add a small helper) that assigns the new ApprovalStore and then rebinds/creates approval_coordinator both on the Session (approval_coordinator) and inside runtime_handle (runtime_handle.approval_coordinator) so they point to the new store; also update _hydrate_session() to set both approval_store and the coordinator binding consistently (use the same helper/property to avoid duplicate logic), and ensure any direct assignment like session.approval_store = ... goes through that setter so reads/writes use the replacement store.
🧹 Nitpick comments (1)
coding-agent/.opencode/prompts/tasks/local-daemon-pr2-session-record-runtime-handle.md (1)
44-47: ⚡ Quick winUse the repo’s organized pytest suites in this task packet.
These target commands hard-code
tests/ui/..., which drifts from the repo’s expected pytest organization and makes the task instructions inconsistent with the standard verification flow.As per coding guidelines, "Run pytest for testing with organized test suites: tests/agentkit/, tests/coding_agent/, tests/cli/".
🤖 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/.opencode/prompts/tasks/local-daemon-pr2-session-record-runtime-handle.md` around lines 44 - 47, Update the hard-coded test invocations in the "Target tests" block to use the repo's organized pytest suites instead of specific tests/ui paths: replace the `uv run pytest tests/ui/...` commands with `uv run pytest tests/agentkit/ tests/coding_agent/ tests/cli/ -k "<filters>" -v` (keeping the same -k filters for session/runtime/event semantics), and update the ruff check to run against `src/coding_agent/server/session_manager.py` plus the organized test suite directories instead of `tests/ui/...`; adjust the three command lines shown in the diff so they reference the canonical suites rather than tests/ui paths.
🤖 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.
Outside diff comments:
In `@coding-agent/src/coding_agent/server/session_manager.py`:
- Around line 857-889: Make sure the ApprovalCoordinator is kept in sync
whenever the public approval_store is replaced: change approval_store into a
property with a setter (or add a small helper) that assigns the new
ApprovalStore and then rebinds/creates approval_coordinator both on the Session
(approval_coordinator) and inside runtime_handle
(runtime_handle.approval_coordinator) so they point to the new store; also
update _hydrate_session() to set both approval_store and the coordinator binding
consistently (use the same helper/property to avoid duplicate logic), and ensure
any direct assignment like session.approval_store = ... goes through that setter
so reads/writes use the replacement store.
---
Nitpick comments:
In
`@coding-agent/.opencode/prompts/tasks/local-daemon-pr2-session-record-runtime-handle.md`:
- Around line 44-47: Update the hard-coded test invocations in the "Target
tests" block to use the repo's organized pytest suites instead of specific
tests/ui paths: replace the `uv run pytest tests/ui/...` commands with `uv run
pytest tests/agentkit/ tests/coding_agent/ tests/cli/ -k "<filters>" -v`
(keeping the same -k filters for session/runtime/event semantics), and update
the ruff check to run against `src/coding_agent/server/session_manager.py` plus
the organized test suite directories instead of `tests/ui/...`; adjust the three
command lines shown in the diff so they reference the canonical suites rather
than tests/ui paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b6f1fdd8-9506-4451-a058-88a75d6390f7
📒 Files selected for processing (4)
coding-agent/.opencode/prompts/tasks/local-daemon-pr2-session-record-runtime-handle.mdcoding-agent/docs/adr/0058-local-daemon-control-plane-executor-architecture.mdcoding-agent/src/coding_agent/server/session_manager.pycoding-agent/tests/ui/test_session_persistence.py
Summary
Tests
Summary by CodeRabbit
Chores
Tests