feat(memory): expand working-memory event semantics#567
Merged
Conversation
Contributor
WalkthroughExtends working-memory event taxonomy with new event types, updates prompts/docs and tool JSON schema, adds conversational prefix classification/formatting in the agent channel, makes SQL event ordering deterministic, changes tool emissions and task-store update flows, and updates tests across memory, tools, agent, and tasks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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)
src/memory/working.rs (1)
887-896:⚠️ Potential issue | 🟠 MajorThe activity-map topic-hint query no longer matches all branch results.
This still hard-codes
event_type = 'branch_completed', butsrc/agent/channel.rs, Lines 3291-3303 now stores prefixed branch conclusions asOutcome,BlockedOn,Constraint, orDeadlineSet. Any channel whose latest branch result uses one of those prefixes will lose its topic hint entirely until this query distinguishes branch-originated events from worker events and includes the new branch terminal types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/memory/working.rs` around lines 887 - 896, The query in working.rs that builds the recent-branch-per-channel window (the variable named query) currently filters only event_type = 'branch_completed' and therefore misses branch terminal events now stored with prefixes (Outcome, BlockedOn, Constraint, DeadlineSet); update the WHERE clause to either (1) match all branch-originated terminal events by including those event_type values (e.g., IN ('branch_completed','Outcome','BlockedOn','Constraint','DeadlineSet')) or (better) use the column that denotes the event origin/source (if present) to restrict to branch-originated events while allowing any terminal event_type; adjust the query construction in the same function/variable to include those additional event types or the origin check so the ROW_NUMBER window returns the true latest branch result per channel.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/channel.rs`:
- Around line 3291-3303: The code is recording cancelled branches as
BranchCompleted because classify_conversational_event_summary is called before
detecting a cancellation; change the logic in the block that handles
BranchResult (where you call classify_conversational_event_summary,
format_conversational_event_summary, and self.deps.working_memory.emit) to first
inspect the BranchResult conclusion (emitted by cancel_branch_with_reason, which
prefixes with "Branch cancelled:") and, if it starts with that prefix, map the
event to a cancellation/error type (e.g., use
WorkingMemoryEventType::BranchCancelled or an appropriate error variant) and
format the summary without giving it a "completed" conversational prefix; then
call self.deps.working_memory.emit with that corrected event_type and formatted
summary (still using format_conversational_event_summary when appropriate) so
cancellations are not recorded as completed.
In `@src/tools/task_update.rs`:
- Around line 158-203: The current code reads previous_status via
self.task_store.get_by_number / get_by_worker_id before calling update(), which
allows races that produce duplicate terminal Outcome events; instead move the
old/new status comparison into the same transactional update path that writes
the task (e.g., change the TaskStore::update implementation to SELECT ... FOR
UPDATE or perform the update in a single DB transaction and return both previous
and new status), then use that returned (previous_status, new_status) to decide
emitting Outcome; update both the Branch and Worker branches (previous_status
usage and the worker validation around TaskUpdateScope::Branch /
TaskUpdateScope::Worker and the args checks) so the status comparison is
performed atomically inside the update routine rather than by a separate
get_by_number / get_by_worker_id call (also apply same fix for the code around
lines cited 250-267).
---
Outside diff comments:
In `@src/memory/working.rs`:
- Around line 887-896: The query in working.rs that builds the
recent-branch-per-channel window (the variable named query) currently filters
only event_type = 'branch_completed' and therefore misses branch terminal events
now stored with prefixes (Outcome, BlockedOn, Constraint, DeadlineSet); update
the WHERE clause to either (1) match all branch-originated terminal events by
including those event_type values (e.g., IN
('branch_completed','Outcome','BlockedOn','Constraint','DeadlineSet')) or
(better) use the column that denotes the event origin/source (if present) to
restrict to branch-originated events while allowing any terminal event_type;
adjust the query construction in the same function/variable to include those
additional event types or the origin check so the ROW_NUMBER window returns the
true latest branch result per channel.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80f44010-bf77-4ca1-a305-c349aa4679cc
📒 Files selected for processing (11)
docs/design-docs/working-memory-implementation-plan.mddocs/design-docs/working-memory.mdprompts/en/memory_persistence.md.j2src/agent/channel.rssrc/memory/working.rssrc/tasks/store.rssrc/tools/memory_persistence_complete.rssrc/tools/send_agent_message.rssrc/tools/spawn_worker.rssrc/tools/task_create.rssrc/tools/task_update.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/agent/channel.rs (1)
196-207:⚠️ Potential issue | 🟡 MinorHandle
Branch cancelled.as a cancellation too.
cancel_branch_with_reason()still emitsBranch cancelled.when the reason is empty (Lines 531-536), but these checks only recognizeBranch cancelled:. In that pathbranch_successstays true and working memory falls back toBranchCompleted, so empty-reason cancels are still recorded as successful completions. Normalize both shapes before deriving success/event type.Also applies to: 3302-3319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 196 - 207, branch_working_memory_event_summary currently only recognizes BRANCH_CANCELLED_PREFIX with a colon, so "Branch cancelled." (emitted by cancel_branch_with_reason when reason is empty) is misclassified as success; modify branch_working_memory_event_summary to normalize both "Branch cancelled:" and "Branch cancelled." shapes before extracting the reason (e.g., check strip_prefix for both BRANCH_CANCELLED_PREFIX variants or strip a trailing ':' or '.' after matching), then trim and truncate the resulting reason and produce the Error/summary branch as before; ensure the same normalization logic is applied wherever branch cancellation is detected (references: branch_working_memory_event_summary and cancel_branch_with_reason).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tasks/store.rs`:
- Around line 431-454: The current lookup in src/tasks/store.rs uses a
worker-only query sorted by updated_at which can return stale rows when
worker_id is non-unique; instead first query the tasks table for the exact
(worker_id, task_number) row and handle success (use task_from_row and proceed),
and only if that exact row is missing do a second query for any row with
worker_id to decide between NotAssigned and WrongTask; update the logic around
the sqlx::query call, the bind usage for worker_id and task_number, and the
return paths that produce WorkerTaskUpdateResult::{NotAssigned, WrongTask} to
rely on the exact-match result rather than the updated_at-sorted worker-only
result.
---
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 196-207: branch_working_memory_event_summary currently only
recognizes BRANCH_CANCELLED_PREFIX with a colon, so "Branch cancelled." (emitted
by cancel_branch_with_reason when reason is empty) is misclassified as success;
modify branch_working_memory_event_summary to normalize both "Branch cancelled:"
and "Branch cancelled." shapes before extracting the reason (e.g., check
strip_prefix for both BRANCH_CANCELLED_PREFIX variants or strip a trailing ':'
or '.' after matching), then trim and truncate the resulting reason and produce
the Error/summary branch as before; ensure the same normalization logic is
applied wherever branch cancellation is detected (references:
branch_working_memory_event_summary and cancel_branch_with_reason).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c6cf52d-9b6e-4906-8711-0a609cd80036
📒 Files selected for processing (5)
src/agent/channel.rssrc/memory/working.rssrc/tasks.rssrc/tasks/store.rssrc/tools/task_update.rs
✅ Files skipped from review due to trivial changes (2)
- src/tasks.rs
- src/memory/working.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/tasks/store.rs (1)
807-855: Avoid hand-maintaining a second copy of the task schema in tests.
setup_test_store()now embeds thetasksandtask_number_seqDDL inline. The next migration-only constraint or index change can leave these tests green against a database shape production never uses. Prefer bootstrapping the in-memory database through the real task migration path instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tasks/store.rs` around lines 807 - 855, Replace the hand-maintained inline DDL in setup_test_store() with the project's canonical migration runner so the in-memory test DB matches production schema; locate setup_test_store() and remove the sqlx::query(...) blocks that create the tasks and task_number_seq tables and the seed insert, then invoke the existing migration path (e.g. call the project's migration function or sqlx::migrate!()/MIGRATOR runner used elsewhere) against the SqlitePool so migrations create the tasks table, task_number_seq, and seed values instead of embedding them here.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/tasks/store.rs`:
- Around line 807-855: Replace the hand-maintained inline DDL in
setup_test_store() with the project's canonical migration runner so the
in-memory test DB matches production schema; locate setup_test_store() and
remove the sqlx::query(...) blocks that create the tasks and task_number_seq
tables and the seed insert, then invoke the existing migration path (e.g. call
the project's migration function or sqlx::migrate!()/MIGRATOR runner used
elsewhere) against the SqlitePool so migrations create the tasks table,
task_number_seq, and seed values instead of embedding them here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b19599a7-37d5-407d-9409-77d94fbf4b7a
📒 Files selected for processing (2)
src/agent/channel.rssrc/tasks/store.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/agent/channel.rs
jamiepine
approved these changes
Apr 20, 2026
Member
jamiepine
left a comment
jamiepine
left a comment
There was a problem hiding this comment.
LGTM. Taxonomy extension is clean, atomic status transitions look right (BEGIN IMMEDIATE + SELECT/UPDATE in same tx), branch cancellation handling covers both shapes, outside-diff topic-hint query disambiguates branch vs worker via summary prefix. Test coverage is solid.
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
2 participants
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
Testing
cargo test -p spacebot task_update -- --test-threads=1 --nocapturejust preflightjust gate-prNotes
main.task_update.Note
AI Summary: This PR expands the working-memory event system to capture structured conversational semantics. The core addition is a new
WorkingMemoryEventTypeenum with 17 event types covering outcomes, blockers, deadlines, constraints, and errors. Events are auto-classified from branch/worker summaries when they use explicit prefixes (e.g., "Outcome: ..."), enabling the memory system to track not just facts but the state and progress of work. The task_update tool now validates worker ownership before global task lookups, closing a security/authorization gap. Key files:src/memory/working.rs(event types + store),src/tools/task_update.rs(worker assignment validation), and updated memory docs.Written by Tembo for commit 0af6765. This will update automatically on new commits.