feat(memory): expand conversational event semantics#524
feat(memory): expand conversational event semantics#524vsumner wants to merge 3 commits intocodex/memory-observation-p1from
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/agent/channel.rs (1)
3870-3898: Consider adding test coverage forconstraint:prefix and edge cases.The tests cover
outcome:andblocked_on:extraction plus fallback behavior, but don't verify:
constraint:prefix extraction (the third recognized prefix)- Case insensitivity (e.g.,
"OUTCOME: ..."or"Outcome: ...")- Empty content after prefix (e.g.,
"outcome:"or"outcome: ")🧪 Suggested additional tests
+ #[test] + fn conversational_event_summary_extracts_constraint_prefix() { + let (event_type, summary) = classify_conversational_event_summary( + "constraint: must not exceed rate limit", + WorkingMemoryEventType::BranchCompleted, + ); + assert_eq!(event_type, WorkingMemoryEventType::Constraint); + assert_eq!(summary, "must not exceed rate limit"); + } + + #[test] + fn conversational_event_summary_is_case_insensitive() { + let (event_type, _) = classify_conversational_event_summary( + "OUTCOME: done", + WorkingMemoryEventType::WorkerCompleted, + ); + assert_eq!(event_type, WorkingMemoryEventType::Outcome); + + let (event_type, _) = classify_conversational_event_summary( + "Blocked_On: waiting", + WorkingMemoryEventType::WorkerCompleted, + ); + assert_eq!(event_type, WorkingMemoryEventType::BlockedOn); + } + + #[test] + fn conversational_event_summary_handles_empty_content_after_prefix() { + let (event_type, summary) = classify_conversational_event_summary( + "outcome:", + WorkingMemoryEventType::WorkerCompleted, + ); + assert_eq!(event_type, WorkingMemoryEventType::Outcome); + assert_eq!(summary, "outcome:"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 3870 - 3898, Add unit tests for classify_conversational_event_summary to cover the missing `constraint:` prefix, case-insensitive prefixes, and empty-content edge cases: add a test that passes a string starting with `"constraint: ..."` and assert the returned event_type is WorkingMemoryEventType::Constraint and summary excludes the prefix; add tests that pass prefixes in different cases (e.g., `"OUTCOME: ...", "Outcome: ..."`) and assert they are normalized to the same WorkingMemoryEventType (e.g., WorkerCompleted -> Outcome) with trimmed summaries; and add tests for inputs like `"outcome:"` and `"outcome: "` asserting the event_type is Outcome and the summary is an empty string (or trimmed-empty) to confirm correct trimming/empty handling in classify_conversational_event_summary.
🤖 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/agent/channel.rs`:
- Around line 3870-3898: Add unit tests for
classify_conversational_event_summary to cover the missing `constraint:` prefix,
case-insensitive prefixes, and empty-content edge cases: add a test that passes
a string starting with `"constraint: ..."` and assert the returned event_type is
WorkingMemoryEventType::Constraint and summary excludes the prefix; add tests
that pass prefixes in different cases (e.g., `"OUTCOME: ...", "Outcome: ..."`)
and assert they are normalized to the same WorkingMemoryEventType (e.g.,
WorkerCompleted -> Outcome) with trimmed summaries; and add tests for inputs
like `"outcome:"` and `"outcome: "` asserting the event_type is Outcome and
the summary is an empty string (or trimmed-empty) to confirm correct
trimming/empty handling in classify_conversational_event_summary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71c3bbb2-b19f-4852-924c-9bf0f433f5b1
📒 Files selected for processing (8)
prompts/en/memory_persistence.md.j2src/agent/channel.rssrc/memory/working.rssrc/tools/memory_persistence_complete.rssrc/tools/send_agent_message.rssrc/tools/spawn_worker.rssrc/tools/task_create.rssrc/tools/task_update.rs
24d18ba to
3e8dced
Compare
Why
PR 1 introduced stricter memory discipline and the first conversational event types, but the working-memory layer still collapsed too many runtime outcomes into generic completion text. That left the system with weaker temporal recall for the exact cases we care about in long agent sessions: blocked work, explicit constraints, terminal outcomes, and concrete deadlines.
This PR finishes that next slice by making those states first-class working-memory events and aligning the persistence prompt/tool schema with the runtime taxonomy. The goal is not to add the observation log yet. The goal is to make the current working-memory surface semantically better before adding another memory layer.
Scope
This PR is intentionally limited to the rest of P2 on the existing working-memory path:
This PR does not introduce the observation-log MVP or prompt assembly refactor yet.
What Changed
deadline_set,blocked_on,constraint, andoutcome.outcome:,blocked_on:, andconstraint:prefixes into typed conversational events instead of leaving them as generic completion strings.Outcomeonly when the task actually reachesdone; non-terminal changes remainTaskUpdate.BlockedOn, and cross-agent delegation now emits a realOutcomeevent rather than dead metadata embedded in anAgentMessagesummary.memory_persistence_completeschema now teach and accept the full conversational event taxonomy so extracted events match the runtime shapes.Testing
cargo fmt --allcargo test -p spacebot emits_outcome -- --test-threads=1 --nocapturecargo test -p spacebot test_event_type_roundtrip -- --test-threads=1 --nocapturecargo test -p spacebot conversational_event_summary_ -- --test-threads=1 --nocapturecargo test -p spacebot persists_conversational_events -- --test-threads=1 --nocapturejust preflightjust gate-prNotes
codex/memory-observation-p1. After PR 522 merges, retarget this PR tomain.send_agent_message,task_create, andtask_updatenow have direct emitter tests because the earlier coverage only proved taxonomy/classifier plumbing, not the actual tool-level event emissions.