Repository navigation
feat(acp): use sqlite local durability - #556
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 48 minutes and 12 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 (12)
📝 WalkthroughWalkthroughThis PR introduces SQLite-backed session ownership with fencing-token lease management for local ACP/CLI durability. It implements a multi-layer feature: a new ChangesLocal Session Ownership Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
delete-cloud
force-pushed
the
feat/acp-sqlite-local-durability
branch
from
June 5, 2026 05:20
08943e6 to
3f1190e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
coding-agent/tests/cli/test_entrypoint_contract.py (1)
149-226: ⚡ Quick winRemove redundant imports inside the test function.
CliRunneris already imported at line 10 andmainis already imported at line 14 (asfrom coding_agent.__main__ import main). The local re-imports on lines 152 and 155 are unnecessary.♻️ Remove redundant imports
def test_acp_stdio_disables_agentkit_tracing( monkeypatch: pytest.MonkeyPatch, ) -> None: - from click.testing import CliRunner - from coding_agent.cli import acp_command - from coding_agent.cli.main import main calls: list[tuple[str, object]] = []🤖 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/cli/test_entrypoint_contract.py` around lines 149 - 226, In test_acp_stdio_disables_agentkit_tracing remove the two redundant local imports "from click.testing import CliRunner" and "from coding_agent.cli.main import main" and rely on the existing module-level imports (CliRunner and main) already imported earlier; keep the test's use of acp_command and all monkeypatches the same, just delete those two local import statements so there are no duplicate imports.coding-agent/tests/cli/test_local_runtime.py (1)
110-125: 💤 Low valueConsider using deterministic synchronization instead of timing-based sleep.
The test relies on
asyncio.sleep(0.01)with a 1ms renewal interval to assert that renewals occurred. Under CI load or slow environments, this could be flaky. Consider using anasyncio.Eventor counter-based wait to make the test deterministic.♻️ Optional: Make renewal assertion deterministic
async def test_server_backed_local_cli_renews_owner_leases_until_close() -> None: manager = object.__new__(ServerBackedLocalCliSessionManager) delegate = FakeDelegate() manager._delegate = delegate manager.owner_store = object() manager.owner_renew_interval_seconds = 0.001 manager._owner_renew_task = None await manager.start_owner_lease_renewal() - await asyncio.sleep(0.01) + # Wait until at least one renewal has occurred + for _ in range(100): + if delegate.renew_owner_leases_called > 0: + break + await asyncio.sleep(0.001) await manager.close() assert delegate.renew_owner_leases_called > 0🤖 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/cli/test_local_runtime.py` around lines 110 - 125, Replace the timing-based sleep with deterministic synchronization: modify the FakeDelegate used in test_server_backed_local_cli_renews_owner_leases_until_close so that its renew_owner_leases implementation signals an asyncio.Event (or increments an awaitable counter and sets an Event when >0), then in the test await that Event with a short timeout after calling manager.start_owner_lease_renewal(), proceed to manager.close(), and assert the same conditions (delegate.renew_owner_leases_called > 0, manager._owner_renew_task is None, delegate.release_owned_sessions_called is True, delegate.close_called is True); reference FakeDelegate, ServerBackedLocalCliSessionManager.start_owner_lease_renewal, and manager.close when making the change.
🤖 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.
Nitpick comments:
In `@coding-agent/tests/cli/test_entrypoint_contract.py`:
- Around line 149-226: In test_acp_stdio_disables_agentkit_tracing remove the
two redundant local imports "from click.testing import CliRunner" and "from
coding_agent.cli.main import main" and rely on the existing module-level imports
(CliRunner and main) already imported earlier; keep the test's use of
acp_command and all monkeypatches the same, just delete those two local import
statements so there are no duplicate imports.
In `@coding-agent/tests/cli/test_local_runtime.py`:
- Around line 110-125: Replace the timing-based sleep with deterministic
synchronization: modify the FakeDelegate used in
test_server_backed_local_cli_renews_owner_leases_until_close so that its
renew_owner_leases implementation signals an asyncio.Event (or increments an
awaitable counter and sets an Event when >0), then in the test await that Event
with a short timeout after calling manager.start_owner_lease_renewal(), proceed
to manager.close(), and assert the same conditions
(delegate.renew_owner_leases_called > 0, manager._owner_renew_task is None,
delegate.release_owned_sessions_called is True, delegate.close_called is True);
reference FakeDelegate,
ServerBackedLocalCliSessionManager.start_owner_lease_renewal, and manager.close
when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ee5d5664-ad47-4935-a457-12c8ed5acd1c
📒 Files selected for processing (11)
coding-agent/docs/adr/0067-local-sqlite-durable-tape-runtime.mdcoding-agent/src/coding_agent/acp/server.pycoding-agent/src/coding_agent/cli/acp_command.pycoding-agent/src/coding_agent/cli/local_runtime.pycoding-agent/src/coding_agent/server/session_manager.pycoding-agent/src/coding_agent/server/stores/session_owner_store.pycoding-agent/tests/acp/test_compat_harness.pycoding-agent/tests/acp/test_server.pycoding-agent/tests/cli/test_entrypoint_contract.pycoding-agent/tests/cli/test_local_runtime.pycoding-agent/tests/ui/test_session_owner_store.py
Add ADR-0067 for SQLite-first local ACP storage, wire ACP stdio to SQLite tape/checkpoint/runtime stores, and add a local SQLite session owner store for owner/fencing enforcement.\n\nVerification:\n- uv run pytest tests/ui/test_session_owner_store.py tests/cli/test_local_runtime.py tests/cli/test_entrypoint_contract.py tests/acp tests/ui/test_session_manager_owner_checks.py tests/coding_agent/test_sqlite_runtime_store.py tests/agentkit/storage/test_sqlite.py -q\n- uv run ruff check src/coding_agent/server/stores/session_owner_store.py src/coding_agent/cli/acp_command.py src/coding_agent/cli/local_runtime.py tests/ui/test_session_owner_store.py tests/cli/test_entrypoint_contract.py tests/cli/test_local_runtime.py tests/acp/test_server.py\n- uv run ruff format --check src/coding_agent/server/stores/session_owner_store.py src/coding_agent/cli/acp_command.py src/coding_agent/cli/local_runtime.py tests/ui/test_session_owner_store.py tests/cli/test_entrypoint_contract.py tests/cli/test_local_runtime.py tests/acp/test_server.py\n- git diff --check
delete-cloud
force-pushed
the
feat/acp-sqlite-local-durability
branch
from
June 5, 2026 05:25
3f1190e to
dbe1cfa
Compare
This was referenced Jun 5, 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
Tests
Summary by CodeRabbit
Release Notes
New Features
Tests