Repository navigation
fix(memory): scope review transitions by session - #628
Conversation
|
Warning Review limit reached
More reviews will be available in 44 minutes and 56 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. 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 (2)
📝 WalkthroughWalkthroughMemory candidates now include session, tape, and profile provenance. Reviewed-memory documents, sync, recall, and review transitions all use session-scoped identity. Build-context hook calls now adapt their arguments from hook signatures. ChangesSession-scoped memory review and sync
Build-context hook dispatch
Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpServer as http_server.py
participant MemoryStore as MemoryReviewStore
participant MemoryHelpers as memory_candidate_belongs_to_session
Client->>HttpServer: POST /sessions/{session_id}/memory/reviews/{candidate_id}
HttpServer->>MemoryStore: load_memory_for_session(session_id, candidate_id)
MemoryStore-->>HttpServer: reviewed memory record or missing
HttpServer->>MemoryHelpers: verify candidate ownership
MemoryHelpers-->>HttpServer: belongs or not belongs
HttpServer->>MemoryStore: accept/reject/archive_candidate_for_session(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
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 `@coding-agent/src/coding_agent/server/http_server.py`:
- Around line 2693-2697: The transition logic is still keying lookup and
mutation by candidate_id alone, then checking session ownership afterward, which
can allow cross-session shadowing or stale prevalidation in a shared review
store. Update the session-scoped transition path in the http_server flow around
the memory candidate handling to use a composite store key like (session_id,
candidate_id), or ensure the validated record is rechecked inside the actual
mutation step before any state change. Apply the same fix to the later memory
candidate transition block referenced by the review comment so both paths are
atomic and session-bound.
In `@coding-agent/src/coding_agent/topics/semantic_index.py`:
- Around line 248-251: The reviewed-memory semantic key is still based only on
the candidate id, so it can collide across sessions even though the metadata is
session-scoped. Update the reviewed-memory ID generation in
SemanticDocId.for_reviewed_memory(record) (or the call site in
semantic_index.py) to include session_id and profile, or otherwise namespace the
backend key by session, so each session writes to a distinct
SemanticMemoryDocument memory_id.
🪄 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: 09137d5c-574f-4c15-89c1-9db40f966e3e
📒 Files selected for processing (7)
coding-agent/src/coding_agent/server/http_server.pycoding-agent/src/coding_agent/topics/memory.pycoding-agent/src/coding_agent/topics/semantic_index.pycoding-agent/src/coding_agent/topics/semantic_sync.pycoding-agent/tests/coding_agent/test_semantic_sync.pycoding-agent/tests/coding_agent/test_topic_memory.pycoding-agent/tests/ui/test_http_server.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
coding-agent/tests/coding_agent/plugins/test_semantic_memory.py (1)
177-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one pipeline-level regression for ctx injection.
This verifies
SemanticMemoryPlugin.build_context()oncectxis already passed in, but it does not exercisePipeline._call_build_context_hooks(), which is the dispatch path changed in this PR. A small integration test that runs abuild_contexthook through the pipeline would catch regressions in_build_context_hook_kwargs().🤖 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/coding_agent/plugins/test_semantic_memory.py` around lines 177 - 180, Add a pipeline-level regression test for ctx injection by exercising Pipeline._call_build_context_hooks() instead of calling SemanticMemoryPlugin.build_context() directly. Update the existing test in test_semantic_memory.py or add a small integration test that runs a build_context hook through Pipeline so the dispatched kwargs path in _build_context_hook_kwargs() is covered and ctx is verified to be injected correctly.
🤖 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/src/coding_agent/plugins/semantic_memory.py`:
- Around line 77-92: The session-scoped memory lookup is incorrectly falling
back to tape.tape_id in _session_id_from_context_or_tape(), which causes
_accepted_memories_for_session() and the SemanticRecallPlanner path in
semantic_memory.py to miss valid session memories. Update the
build_context/planning flow to require an explicit ctx.session_id when using
_accepted_memories_for_session(), or add a separate legacy/unscoped branch when
no session_id is present, and apply the same fix to the other referenced call
sites using the same session lookup logic.
---
Nitpick comments:
In `@coding-agent/tests/coding_agent/plugins/test_semantic_memory.py`:
- Around line 177-180: Add a pipeline-level regression test for ctx injection by
exercising Pipeline._call_build_context_hooks() instead of calling
SemanticMemoryPlugin.build_context() directly. Update the existing test in
test_semantic_memory.py or add a small integration test that runs a
build_context hook through Pipeline so the dispatched kwargs path in
_build_context_hook_kwargs() is covered and ctx is verified to be injected
correctly.
🪄 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: 55a045dd-c6ab-431a-90e6-13bce8c8375a
📒 Files selected for processing (11)
coding-agent/src/agentkit/runtime/pipeline.pycoding-agent/src/coding_agent/plugins/semantic_memory.pycoding-agent/src/coding_agent/server/http_server.pycoding-agent/src/coding_agent/topics/memory.pycoding-agent/src/coding_agent/topics/semantic_index.pycoding-agent/src/coding_agent/topics/semantic_recall.pycoding-agent/tests/coding_agent/plugins/test_semantic_memory.pycoding-agent/tests/coding_agent/test_memory_review.pycoding-agent/tests/coding_agent/test_semantic_recall.pycoding-agent/tests/coding_agent/test_semantic_sync.pycoding-agent/tests/ui/test_http_server.py
🚧 Files skipped from review as they are similar to previous changes (3)
- coding-agent/src/coding_agent/server/http_server.py
- coding-agent/tests/ui/test_http_server.py
- coding-agent/tests/coding_agent/test_semantic_sync.py
Summary
Verification
UV_PYTHON=3.12 uv run pytest tests/coding_agent/test_memory_review.py tests/coding_agent/test_topic_memory.py tests/coding_agent/test_semantic_sync.py tests/coding_agent/test_memory_switch.py tests/coding_agent/test_memory_index_backend_contract.py tests/ui/test_http_server.py::TestMemoryReviewTransitions tests/coding_agent/test_semantic_recall.py tests/coding_agent/plugins/test_semantic_memory.py tests/coding_agent/test_recall_context.py tests/coding_agent/test_cross_topic_memory_smoke.py -q-> 112 passeduv run ruff check src/coding_agent/topics/memory.py src/coding_agent/topics/semantic_index.py src/coding_agent/topics/semantic_sync.py src/coding_agent/server/http_server.py tests/coding_agent/test_topic_memory.py tests/coding_agent/test_semantic_sync.py tests/ui/test_http_server.py-> All checks passeduv run ruff format --check src/coding_agent/topics/memory.py src/coding_agent/topics/semantic_index.py src/coding_agent/topics/semantic_sync.py src/coding_agent/server/http_server.py tests/coding_agent/test_topic_memory.py tests/coding_agent/test_semantic_sync.py tests/ui/test_http_server.py-> 7 files already formattedgit diff --check -- src/coding_agent/server/http_server.py src/coding_agent/topics/memory.py src/coding_agent/topics/semantic_index.py src/coding_agent/topics/semantic_sync.py tests/coding_agent/test_semantic_sync.py tests/coding_agent/test_topic_memory.py tests/ui/test_http_server.py-> passReview
tests/ui/test_http_server.py; focused UI transition tests are included aboveNotes
RTK.mdandprompts/codex-delegatee.md, but those files are not present in this worktree; treated as setup risk, not loaded rules.Summary by CodeRabbit