feat: working memory system — temporal awareness for channels#454
Merged
feat: working memory system — temporal awareness for channels#454
Conversation
Replace the timer-based bulletin with a layered context assembly system
that gives channels temporal awareness, cross-channel visibility, and
change-driven knowledge synthesis.
Phases implemented:
1. Event store + emission (10 trigger points across workers, branches,
cron, memory saves, errors, tasks, agent messages, startup)
2. Context injection (render_working_memory + render_channel_activity_map
into channel system prompts via template variables)
3. Knowledge synthesis (dirty-flag + debounce replaces 15-min bulletin
timer, narrower 5-section scope vs 8-section bulletin)
4. Intra-day synthesis + daily summaries (cortex tick loop produces
rolling narrative paragraphs + end-of-day summaries)
5a. Smarter persistence triggers (triple trigger: message count + time +
event density; dual output: memories + working memory events)
5b. Sunset compactor memory extraction (compactor is now a pure
summarizer, no tool server, no memory_save)
Additional fixes:
- Prompt inspect endpoint now renders all dynamic sections (working
memory, channel map, org context, projects, adapter) instead of
passing None — matches what the LLM actually sees
- Channel template reordered: identity → static behavior → static
context → working memory → channel map → knowledge → status
- Stale 'Memory Context' references updated to 'Knowledge Context'
- Bulletin staleness no longer blocks work readiness (change-driven
synthesis has no staleness concept)
Contributor
WalkthroughAdds an append-only, day-scoped working-memory event store with intraday/daily synthesis and rendering, replaces timer-driven bulletin refresh with change-driven knowledge synthesis, wires tools/agents to emit/consume working-memory events, updates prompts/templates, adds config/runtime state, and includes a DB migration for three new tables. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/agents.rs (1)
420-451:⚠️ Potential issue | 🟠 MajorThe timezone fallback is wrong in both warmup and live-create paths.
Both blocks pick one string first and only then parse it. A mistyped
user_timezonetherefore bypasses a validcron_timezoneand falls back to UTC, which skews working-memory day boundaries and intraday summaries.Also applies to: 825-833
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 420 - 451, The timezone fallback currently chooses the first present string then parses it, so a malformed runtime_config.user_timezone can prevent using a valid runtime_config.cron_timezone; update the logic around working_memory_tz (used when constructing WorkingMemoryStore) to attempt to parse runtime_config.user_timezone first and if parsing fails try parsing runtime_config.cron_timezone, and only default to chrono_tz::Tz::UTC if both parses fail; apply the same fix to the equivalent warmup/live-create block (the other occurrence around lines 825-833) so both paths parse and fallback correctly.
🟡 Minor comments (7)
docs/design-docs/working-memory.md-107-107 (1)
107-107:⚠️ Potential issue | 🟡 MinorAdd explicit language tags to fenced code blocks.
Markdown lint flags these fences (MD040). Please annotate each block with a language (for example
text,rust,sql,jinja2) to resolve warnings at Line 107, Line 616, Line 624, Line 660, Line 733, Line 767, Line 784, and Line 860.Also applies to: 616-616, 624-624, 660-660, 733-733, 767-767, 784-784, 860-860
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design-docs/working-memory.md` at line 107, Markdown fenced code blocks in the document that include the table row containing "Link channels | `AgentMessage`" and several other fenced blocks flagged by the linter are missing language annotations; add appropriate language identifiers (e.g., text, rust, sql, jinja2) immediately after the opening triple backticks for each affected fence (the ones around the `AgentMessage` example and the other flagged fences) so they read like ```text or ```rust etc., ensuring each block uses the most suitable language tag to silence MD040.src/api/agents.rs-825-833 (1)
825-833:⚠️ Potential issue | 🟡 MinorEmit the same startup marker for API-created agents.
Boot-time agents get an
Agent startedworking-memory event ininitialize_agents(), but this path never records one. Agents added through the API will have a different temporal baseline until the next restart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 825 - 833, After constructing the agent's WorkingMemoryStore in the API creation path (the working_memory block in src/api/agents.rs), emit the same "Agent started" working-memory event that initialize_agents() logs so API-created agents have the same startup baseline; either call the same helper used by initialize_agents() or invoke the WorkingMemoryStore method used to record events (e.g., record_event / push / insert) with an "Agent started" message and appropriate metadata/timestamp immediately after creating crate::memory::WorkingMemoryStore::new(...) for the new agent.src/main.rs-2715-2726 (1)
2715-2726:⚠️ Potential issue | 🟡 MinorRecord the startup event after initialization fully succeeds.
initialize_agents()still has several fallible stages after this block. If any of them error, working memory will still containAgent startedfor an agent that never actually finished booting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 2715 - 2726, The "Agent started" working-memory emit is being recorded too early—move the agent.deps.working_memory.emit(...).importance(0.3).record() call out of the loop that runs before later fallible stages and instead invoke it only after initialize_agents() (and any subsequent initialization steps) complete successfully; locate the block that currently iterates agents.values() and the emit call and relocate or defer that emit/record to run after initialize_agents() returns Ok so agents only record "Agent started (<id>)" when initialization truly succeeded.src/tools/send_agent_message.rs-292-300 (1)
292-300:⚠️ Potential issue | 🟡 MinorAttach the originating channel to delegated-message events.
This tool already captures
originating_channel, but the new working-memory event drops it. That loses the conversation context this PR is trying to preserve for channel-aware prompts.Suggested fix
if let Some(working_memory) = &self.working_memory { - working_memory + let mut builder = working_memory .emit( crate::memory::WorkingMemoryEventType::AgentMessage, format!("Delegated task #{task_number} to {target_display}"), ) - .importance(0.7) - .record(); + .importance(0.7); + if let Some(channel_id) = &self.originating_channel { + builder = builder.channel(channel_id.clone()); + } + builder.record(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/send_agent_message.rs` around lines 292 - 300, The working-memory event emitted in send_agent_message.rs drops the originating channel; modify the emit call on self.working_memory (the emit invocation for crate::memory::WorkingMemoryEventType::AgentMessage and the format!("Delegated task #{task_number} to {target_display}") payload) to include the originating_channel captured by the tool (e.g., self.originating_channel or the local originating_channel variable) as event metadata or part of the event payload so the event preserves conversation context; use the working memory API for attaching context (metadata/key) rather than embedding it only in the log string.docs/design-docs/working-memory-example-prompt.md-19-24 (1)
19-24:⚠️ Potential issue | 🟡 MinorThis example gives the channel worker-only capabilities.
The live channel prompt routes file edits, shell commands, and web browsing through workers, so describing direct access here makes the “realistic example” inaccurate.
💡 Proposed fix
-Your workspace is at `/home/atlas/workspace`. You can read and write files, run shell commands, browse the web, and spawn coding workers for deep implementation tasks. +Your workers operate in `/home/atlas/workspace`. You can delegate file edits, shell commands, and web research to workers, and spawn coding workers for deep implementation tasks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design-docs/working-memory-example-prompt.md` around lines 19 - 24, The prompt erroneously grants direct capabilities by stating "You can read and write files, run shell commands, browse the web, and spawn coding workers for deep implementation tasks"; replace that sentence so the prompt clarifies that those actions are performed only via the live channel worker (or spawned coding workers) rather than by the assistant directly, and update any wording around the workspace path "/home/atlas/workspace" to indicate it is accessible through worker-mediated operations rather than direct assistant access.src/memory/working.rs-571-595 (1)
571-595:⚠️ Potential issue | 🟡 MinorThe current-channel boost is not actually enforced.
Lines 571-579 truncate the tail by global importance before the loop at Lines 587-595. Low-importance events from
channel_idcan still disappear from this channel's prompt despite the "always include them" comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/memory/working.rs` around lines 571 - 595, The tail truncation can drop low-importance events from the current channel despite the "always include" comment; modify the tail construction so that when unsynthesized.len() > max_tail you first separate events into same_channel (where e.channel_id == channel_id) and others, always include all same_channel events in the tail, then fill remaining slots up to max_tail with highest-importance events from others (use the same importance sort and truncate logic), finally re-sort the resulting tail by timestamp; update references around tail_events, unsynthesized, max_tail, channel_id, format_event_line, estimate_tokens, tokens_used and today_budget so the loop still consumes tokens in chronological order and no duplicate events are included.src/memory/working.rs-979-983 (1)
979-983:⚠️ Potential issue | 🟡 MinorFix in-memory SQLite test pool to use a single connection.
sqlite::memory:scopes the database per connection, so with the defaultSqlitePoolsettings, migrations can run on one connection while later queries hit a separate empty database. This causes test flakiness. UseSqlitePoolOptions::new().max_connections(1)as done in other test helpers throughout the codebase (tasks/store.rs, conversation/channels.rs, agent/cortex.rs, etc.).Suggested fix
- let pool = SqlitePool::connect("sqlite::memory:").await.unwrap(); + let pool = sqlx::sqlite::SqlitePoolOptions::new() + .max_connections(1) + .connect("sqlite::memory:") + .await + .unwrap();Note: The same issue exists in src/projects/store.rs and src/messaging/webchat.rs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/memory/working.rs` around lines 979 - 983, The in-memory SQLite test pool in async fn setup_test_store (returning Arc<WorkingMemoryStore>) uses SqlitePool::connect("sqlite::memory:") which creates per-connection in-memory DBs and causes flaky tests; change to build a single-connection pool using SqlitePoolOptions::new().max_connections(1).connect("sqlite::memory:").await.unwrap(), then run sqlx::migrate! and construct the WorkingMemoryStore as before; apply the same single-connection SqlitePoolOptions fix to the analogous pools in src/projects/store.rs and src/messaging/webchat.rs.
🧹 Nitpick comments (5)
prompts/en/memory_persistence.md.j2 (1)
30-39: Explicitly exclude routine tool activity fromevents.The new guidance defines what to include, but the anti-noise rule below only talks about memories. Add a line telling the model to skip routine tool calls, retries, and internal bookkeeping unless they changed the outcome or were user-visible.
Prompt tweak
4. **Extract events.** While reviewing the conversation, identify key decisions, important events, and errors. Include them in the `events` field of `memory_persistence_complete`: - `event_type`: "decision" for commitments or choices made, "error" for failures or problems, "system" for other notable events - `summary`: one-line description of what happened - `importance`: 0.0-1.0 score (decisions and errors typically 0.6-0.8) + - Do not emit routine tool calls, retries, or bookkeeping unless they materially changed the outcome or were user-visible. - Events feed the agent's temporal working memory — they help the agent remember *what happened today*, not just facts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompts/en/memory_persistence.md.j2` around lines 30 - 39, Update the prompt guidance to explicitly exclude routine tool activity from the `events` list: add a sentence to the instructions around steps 4–5 (near the `events` guidance and the `memory_persistence_complete` requirement) that tells the model to skip routine tool calls, automated retries, and internal bookkeeping as events unless they visibly changed user-facing outcome; reference `events`, `memory_save`, and `memory_persistence_complete` so it's clear this only affects what gets put into `events` and does not change how `memory_save` or the `memory_persistence_complete` outcome/saved IDs are handled.prompts/en/cortex_intraday_synthesis.md.j2 (1)
1-8: Add a no-invention guard to the synthesis prompt.These paragraphs get persisted back into context. The prompt should explicitly say to use only the supplied events and omit unknown actor, channel, or causal details instead of guessing.
Prompt tweak
Summarize the following {{ event_count }} events from {{ time_start }} to {{ time_end }} into a concise 50-100 word narrative paragraph. Focus on: what was accomplished, what decisions were made, what failed, what is in progress. Be specific about who did what and in which channel where relevant. Use present/past tense naturally. Do not list events mechanically — write a narrative. +Use only the supplied events. If an actor, channel, or causal link is not explicit, omit it rather than guessing. Events: {{ events }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prompts/en/cortex_intraday_synthesis.md.j2` around lines 1 - 8, Add a no-invention guard to the synthesis template: update the prompt in cortex_intraday_synthesis.md.j2 to explicitly instruct the model to use only the supplied {{ events }} and not to invent or infer unknown actors, channels, or causal details; tell it to omit any such unknown information rather than guessing. Insert this sentence near the top of the prompt (e.g., after the first instruction line) so the generated 50–100 word narrative paragraph includes only facts present in {{ events }} and leaves out any unspecified actor/channel/causal details.docs/design-docs/working-memory-example-prompt.md (1)
92-165: The[UNCHANGED]tail is already out of sync with the live prompt.It still shows the old memory taxonomy and older channel rules, so this doc stops being a realistic post-rollout example as soon as this PR lands. Consider sourcing this block from
prompts/en/channel.md.j2or clearly labeling it as illustrative pseudocode instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design-docs/working-memory-example-prompt.md` around lines 92 - 165, The `[UNCHANGED from here — these sections remain as they are today]` tail in working-memory-example-prompt.md is stale and duplicates the live channel rules and memory taxonomy; replace that stale block by either injecting the up-to-date content from prompts/en/channel.md.j2 or clearly mark the section as illustrative pseudocode. Locate the `[UNCHANGED ...]` marker in working-memory-example-prompt.md and update the block to reference or include the canonical source (prompts/en/channel.md.j2) or add a prominent “illustrative only” note so readers know it’s not the live prompt.src/agent/channel.rs (1)
3223-3241: Consider the cost of the event density DB query.The
count_events_sincecall hits the database on everycheck_memory_persistenceinvocation when the cheap triggers don't fire. While the guard clause (!message_trigger && !time_trigger) limits when this runs, it could still add latency on busy channels with frequent messages but below the message threshold.The current design is acceptable since:
- The cheap triggers fire first most of the time
- The query only runs when both message and time triggers fail
- A single COUNT query should be fast
If this becomes a bottleneck, consider caching the event count or computing it lazily in the background.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 3223 - 3241, check_memory_persistence currently calls self.deps.working_memory.count_events_since on every invocation when the cheap triggers don't fire, which can add DB latency; update check_memory_persistence to first consult a short-lived in-memory cache (e.g., a struct field like last_event_count: (timestamp, count) or last_event_count_at and cached_event_count) and only call count_events_since when the cache is stale (older than a small TTL), or instead kick off a background task to refresh the cached count periodically; reference the count_events_since call, the check_memory_persistence function, and wm_config.persistence_event_density_threshold and ensure the cached value is used to compare against persistence_event_density_threshold to avoid frequent DB COUNT queries.src/tools/spawn_worker.rs (1)
459-467: Consider bounding task text before persisting it to working memory.Line 463 currently stores the full task string; trimming this to a short preview prevents oversized event rows and prompt bloat when tasks are long.
♻️ Suggested change
+ let task_preview: String = args.task.chars().take(240).collect(); + let task_suffix = if args.task.chars().count() > 240 { "…" } else { "" }; self.deps .working_memory .emit( crate::memory::WorkingMemoryEventType::WorkerSpawned, - format!("Worker spawned (cortex): {}", &args.task), + format!("Worker spawned (cortex): {task_preview}{task_suffix}"), ) .importance(0.5) .record();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/spawn_worker.rs` around lines 459 - 467, The WorkingMemory emit call is persisting the full args.task text; truncate or limit the task string to a short preview before formatting to avoid oversized events and prompt bloat — e.g., create a bounded preview (max N chars) from args.task (preserving start and optionally ellipsizing) and use that preview in the format!("Worker spawned (cortex): {}", preview) passed to self.deps.working_memory.emit for WorkingMemoryEventType::WorkerSpawned; keep the rest of the chain (.importance(...).record()) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@prompts/en/channel.md.j2`:
- Around line 176-180: The template currently hides the legacy bulletin when
working_memory exists because the condition uses "memory_bulletin and not
working_memory and not knowledge_synthesis"; change this to only gate on the
absence of synthesized knowledge by updating the Jinja conditional to
"memory_bulletin and not knowledge_synthesis" so the bulletin is shown whenever
there's no knowledge_synthesis regardless of working_memory (refer to the
symbols memory_bulletin, working_memory, and knowledge_synthesis and the
conditional block that renders "## Memory Context").
In `@prompts/en/cortex_knowledge_synthesis.md.j2`:
- Around line 16-21: Update the exclusions in the
prompts/en/cortex_knowledge_synthesis.md.j2 template so that participant-role
facts (e.g., the bullet that currently reads '"The user is the CEO" or similar
role statements (Layer 1: Identity)') are NOT excluded from synthesis until the
Participant Context layer exists; keep excluding agent/company identity entries
but remove or reclassify the participant-role bullet from the exclusion list and
add a short note that participant-role facts should be retained in synthesis
until the participant layer ships.
In `@src/agent/channel_dispatch.rs`:
- Around line 587-596: The code currently writes the raw user-provided task into
working memory via state.deps.working_memory.emit(..., format!("Worker spawned:
{task}")) for WorkingMemoryEventType::WorkerSpawned (and similarly at the other
occurrence), which can persist secrets/PII; change it to emit a scrubbed,
truncated summary instead of the raw task by running the incoming task through a
sanitization/summary helper (e.g., redact_sensitive_and_truncate or
summarize_for_working_memory) before formatting the event string, so the emit
call contains something like "Worker spawned: {scrubbed_summary}" rather than
the verbatim task. Ensure the same fix is applied to the other occurrence
referenced in the review.
In `@src/agent/cortex.rs`:
- Around line 2607-2614: The code reads knowledge_synthesis_version again after
the async LLM/synthesis work, which can race with concurrent writes and
incorrectly advance knowledge_synthesis_last_version; instead, snapshot the
current version before the async work by loading
deps.runtime_config.knowledge_synthesis_version into a local variable (e.g.,
saved_version) prior to calling gather_sections_from_list or making the LLM
request, and then store that saved_version into
deps.runtime_config.knowledge_synthesis_last_version when marking the result as
synthesized so the stored version reflects the version the output was actually
based on.
- Around line 2094-2106: The dirty-flag refresh spawns
generate_knowledge_synthesis() without taking the same synthesis mutex used by
the warmup path, allowing concurrent syntheses; change the refresh_task creation
so the spawned task first acquires the existing synthesis mutex (the same mutex
used by the warmup/warmup lock logic) before calling
generate_knowledge_synthesis(&deps, &synthesis_logger). Move/clone the synthesis
mutex into the async move closure and call .lock().await (or the equivalent
guard) inside the spawned task so the generate_knowledge_synthesis call and
subsequent writes to knowledge_synthesis, memory_bulletin, and version trackers
are serialized with the warmup path.
- Around line 2888-2916: The code currently drops raw unsynthesized events from
yesterday; change the flow so any raw events are rolled into the daily summary
instead of being ignored. Specifically, call wm.get_events_for_day(&yesterday)
whenever building the daily summary (both when intraday.is_empty() and when
intraday is non-empty), and if events are returned, convert them into one or
more summary paragraphs (e.g., a single "Unsynthesized events" paragraph
including the event count/text) and append that paragraph to blocks_text and add
to total_events before saving the summary; update the logic around
get_intraday_syntheses, get_events_for_day, blocks_text, and total_events so
unsynthesized tails are included rather than returning early.
- Around line 1955-1958: The version bump only runs when report.merged > 0, but
generate_knowledge_synthesis() also depends on memory set and importance
ordering, so changes from pruning or decay must trigger the same bump; update
the branch in cortex.rs (referencing report.merged and
cortex.deps.runtime_config.bump_knowledge_synthesis_version()) to also call
bump_knowledge_synthesis_version() when report.pruned > 0 or report.decayed > 0
(i.e., bump if any of merged, pruned, or decayed are > 0) so cached synthesis is
invalidated after maintenance runs.
- Around line 2158-2166: The main loop must not await LLM work inline: instead
of awaiting maybe_synthesize_intraday_batch(&cortex.deps, logger).await and
maybe_synthesize_daily_summary(&cortex.deps, logger).await directly inside the
loop, kick those off as background tasks (e.g., tokio::spawn) so the select!
continues draining event_rx and memory_event_rx; capture the JoinHandle and log
any returned Err from the spawned future (or detach and have the spawned task
log its own errors using tracing::warn!(%error,...)). Keep passing cortex.deps
and logger into the spawned closure (or clone handles as needed) and ensure the
spawned task performs the same error handling currently done inline.
- Around line 900-903: The use of matches!(event, ProcessEvent::MemorySaved { ..
}) moves the non-Copy ProcessEvent and prevents reusing event later in
signal_from_event(...); change the check to borrow event instead (e.g.,
matches!(&event, ProcessEvent::MemorySaved { .. }) or use if let
ProcessEvent::MemorySaved { .. } = &event) so that
self.deps.runtime_config.bump_knowledge_synthesis_version() still runs but event
remains available for subsequent calls like signal_from_event(...).
- Around line 2576-2579: Add an explicit .default_max_turns(1) to each of the
three AgentBuilder chains (the builder that uses the loaded
cortex_knowledge_synthesis preamble and the two builders that currently inline
preambles) and move any inline preamble string constants into prompt files and
load them via prompt_engine (use the loaded prompt in the builder call instead
of the inline string). Concretely: for the builder that currently calls
.preamble(&synthesis_preamble).hook(CortexHook::new()).build() add
.default_max_turns(1) on the chain; for the two builders that currently use
inline preamble text, create or reuse prompt entries (e.g., prompts for the
corresponding preambles), load them via prompt_engine, pass the loaded prompt to
.preamble(...) or .prompt(...) as appropriate, and also add
.default_max_turns(1) to those builder chains so each one-shot agent is
explicitly limited to one turn.
In `@src/api/channels.rs`:
- Around line 467-649: The inspect_prompt path is silently swallowing errors by
using unwrap_or_default() and .ok() on calls like render_working_memory,
render_channel_activity_map, channel_state.channel_store.list_active,
prompt_engine.render_available_channels, prompt_engine.render_org_context,
store.list_projects, store.list_repos, store.list_worktrees_with_repos, and
prompt_engine.render_projects_context; change these to surface failures (return
an Err or a descriptive error response) or at minimum log the error and
propagate it up instead of converting to empty values—replace
unwrap_or_default()/.ok() with proper ? propagation or map_err logging and then
return a failure from inspect_prompt so callers see template/db/render errors
rather than omitted sections.
In `@src/cron/scheduler.rs`:
- Around line 378-386: The code is persisting full error text into working
memory via exec_context.deps.working_memory.emit with
WorkingMemoryEventType::Error using exec_job_id and error; change this to log
the full error (retain existing logs) but only emit a redacted/summary string to
working memory (e.g., "Cron failed: {exec_job_id}: internal error" or a short
sanitized message), ensuring exec_context.deps.working_memory.emit gets the safe
summary and not the raw error payload while keeping the detailed error in
existing logging paths.
In `@src/main.rs`:
- Around line 2548-2559: The timezone fallback currently picks
agent_config.user_timezone.or(agent_config.cron_timezone) before parsing, so an
invalid user_timezone prevents trying a valid cron_timezone; change logic used
to compute working_memory_timezone to parse each option separately: attempt to
parse agent_config.user_timezone (if Some) and if that parse fails, then attempt
to parse agent_config.cron_timezone, and only then fall back to
chrono_tz::Tz::UTC; update the code that constructs working_memory_timezone (the
block feeding WorkingMemoryStore::new) to reflect this sequential-parse fallback
so WorkingMemoryStore::new still receives a valid chrono_tz::Tz.
In `@src/memory/working.rs`:
- Around line 735-739: The code in render_channel_activity_map() slices
topic_summary by byte index which can split multibyte UTF-8 chars and panic;
replace the byte-slice logic for truncated with a safe char-aware truncation,
e.g. build truncated via topic_summary.chars().take(80).collect::<String>() and
append "..." only when the character count exceeded 80, ensuring you use the
existing topic_summary variable and preserve the same behavior of adding
ellipsis.
In `@src/tools/task_update.rs`:
- Around line 236-246: The current working_memory.emit call always logs "Task
#{} updated to {}" using updated.status, which misrepresents edits; update the
code around working_memory.emit (the emit call for
crate::memory::WorkingMemoryEventType::TaskUpdate) to compute the actual delta
between the previous and new task (compare fields like title, description,
status, subtasks, approvals, metadata, etc.), then emit a concise message
describing what changed (e.g., "Task `#N` title changed", "Task `#N` description
edited", "Task `#N` subtask X completed", "Task `#N` metadata merged", or "Task `#N`
no-op") or use more specific event types if available; reference
updated.task_number and the prior task/state to build the message so the working
memory records the real change instead of always writing the status.
---
Outside diff comments:
In `@src/api/agents.rs`:
- Around line 420-451: The timezone fallback currently chooses the first present
string then parses it, so a malformed runtime_config.user_timezone can prevent
using a valid runtime_config.cron_timezone; update the logic around
working_memory_tz (used when constructing WorkingMemoryStore) to attempt to
parse runtime_config.user_timezone first and if parsing fails try parsing
runtime_config.cron_timezone, and only default to chrono_tz::Tz::UTC if both
parses fail; apply the same fix to the equivalent warmup/live-create block (the
other occurrence around lines 825-833) so both paths parse and fallback
correctly.
---
Minor comments:
In `@docs/design-docs/working-memory-example-prompt.md`:
- Around line 19-24: The prompt erroneously grants direct capabilities by
stating "You can read and write files, run shell commands, browse the web, and
spawn coding workers for deep implementation tasks"; replace that sentence so
the prompt clarifies that those actions are performed only via the live channel
worker (or spawned coding workers) rather than by the assistant directly, and
update any wording around the workspace path "/home/atlas/workspace" to indicate
it is accessible through worker-mediated operations rather than direct assistant
access.
In `@docs/design-docs/working-memory.md`:
- Line 107: Markdown fenced code blocks in the document that include the table
row containing "Link channels | `AgentMessage`" and several other fenced blocks
flagged by the linter are missing language annotations; add appropriate language
identifiers (e.g., text, rust, sql, jinja2) immediately after the opening triple
backticks for each affected fence (the ones around the `AgentMessage` example
and the other flagged fences) so they read like ```text or ```rust etc.,
ensuring each block uses the most suitable language tag to silence MD040.
In `@src/api/agents.rs`:
- Around line 825-833: After constructing the agent's WorkingMemoryStore in the
API creation path (the working_memory block in src/api/agents.rs), emit the same
"Agent started" working-memory event that initialize_agents() logs so
API-created agents have the same startup baseline; either call the same helper
used by initialize_agents() or invoke the WorkingMemoryStore method used to
record events (e.g., record_event / push / insert) with an "Agent started"
message and appropriate metadata/timestamp immediately after creating
crate::memory::WorkingMemoryStore::new(...) for the new agent.
In `@src/main.rs`:
- Around line 2715-2726: The "Agent started" working-memory emit is being
recorded too early—move the
agent.deps.working_memory.emit(...).importance(0.3).record() call out of the
loop that runs before later fallible stages and instead invoke it only after
initialize_agents() (and any subsequent initialization steps) complete
successfully; locate the block that currently iterates agents.values() and the
emit call and relocate or defer that emit/record to run after
initialize_agents() returns Ok so agents only record "Agent started (<id>)" when
initialization truly succeeded.
In `@src/memory/working.rs`:
- Around line 571-595: The tail truncation can drop low-importance events from
the current channel despite the "always include" comment; modify the tail
construction so that when unsynthesized.len() > max_tail you first separate
events into same_channel (where e.channel_id == channel_id) and others, always
include all same_channel events in the tail, then fill remaining slots up to
max_tail with highest-importance events from others (use the same importance
sort and truncate logic), finally re-sort the resulting tail by timestamp;
update references around tail_events, unsynthesized, max_tail, channel_id,
format_event_line, estimate_tokens, tokens_used and today_budget so the loop
still consumes tokens in chronological order and no duplicate events are
included.
- Around line 979-983: The in-memory SQLite test pool in async fn
setup_test_store (returning Arc<WorkingMemoryStore>) uses
SqlitePool::connect("sqlite::memory:") which creates per-connection in-memory
DBs and causes flaky tests; change to build a single-connection pool using
SqlitePoolOptions::new().max_connections(1).connect("sqlite::memory:").await.unwrap(),
then run sqlx::migrate! and construct the WorkingMemoryStore as before; apply
the same single-connection SqlitePoolOptions fix to the analogous pools in
src/projects/store.rs and src/messaging/webchat.rs.
In `@src/tools/send_agent_message.rs`:
- Around line 292-300: The working-memory event emitted in send_agent_message.rs
drops the originating channel; modify the emit call on self.working_memory (the
emit invocation for crate::memory::WorkingMemoryEventType::AgentMessage and the
format!("Delegated task #{task_number} to {target_display}") payload) to include
the originating_channel captured by the tool (e.g., self.originating_channel or
the local originating_channel variable) as event metadata or part of the event
payload so the event preserves conversation context; use the working memory API
for attaching context (metadata/key) rather than embedding it only in the log
string.
---
Nitpick comments:
In `@docs/design-docs/working-memory-example-prompt.md`:
- Around line 92-165: The `[UNCHANGED from here — these sections remain as they
are today]` tail in working-memory-example-prompt.md is stale and duplicates the
live channel rules and memory taxonomy; replace that stale block by either
injecting the up-to-date content from prompts/en/channel.md.j2 or clearly mark
the section as illustrative pseudocode. Locate the `[UNCHANGED ...]` marker in
working-memory-example-prompt.md and update the block to reference or include
the canonical source (prompts/en/channel.md.j2) or add a prominent “illustrative
only” note so readers know it’s not the live prompt.
In `@prompts/en/cortex_intraday_synthesis.md.j2`:
- Around line 1-8: Add a no-invention guard to the synthesis template: update
the prompt in cortex_intraday_synthesis.md.j2 to explicitly instruct the model
to use only the supplied {{ events }} and not to invent or infer unknown actors,
channels, or causal details; tell it to omit any such unknown information rather
than guessing. Insert this sentence near the top of the prompt (e.g., after the
first instruction line) so the generated 50–100 word narrative paragraph
includes only facts present in {{ events }} and leaves out any unspecified
actor/channel/causal details.
In `@prompts/en/memory_persistence.md.j2`:
- Around line 30-39: Update the prompt guidance to explicitly exclude routine
tool activity from the `events` list: add a sentence to the instructions around
steps 4–5 (near the `events` guidance and the `memory_persistence_complete`
requirement) that tells the model to skip routine tool calls, automated retries,
and internal bookkeeping as events unless they visibly changed user-facing
outcome; reference `events`, `memory_save`, and `memory_persistence_complete` so
it's clear this only affects what gets put into `events` and does not change how
`memory_save` or the `memory_persistence_complete` outcome/saved IDs are
handled.
In `@src/agent/channel.rs`:
- Around line 3223-3241: check_memory_persistence currently calls
self.deps.working_memory.count_events_since on every invocation when the cheap
triggers don't fire, which can add DB latency; update check_memory_persistence
to first consult a short-lived in-memory cache (e.g., a struct field like
last_event_count: (timestamp, count) or last_event_count_at and
cached_event_count) and only call count_events_since when the cache is stale
(older than a small TTL), or instead kick off a background task to refresh the
cached count periodically; reference the count_events_since call, the
check_memory_persistence function, and
wm_config.persistence_event_density_threshold and ensure the cached value is
used to compare against persistence_event_density_threshold to avoid frequent DB
COUNT queries.
In `@src/tools/spawn_worker.rs`:
- Around line 459-467: The WorkingMemory emit call is persisting the full
args.task text; truncate or limit the task string to a short preview before
formatting to avoid oversized events and prompt bloat — e.g., create a bounded
preview (max N chars) from args.task (preserving start and optionally
ellipsizing) and use that preview in the format!("Worker spawned (cortex): {}",
preview) passed to self.deps.working_memory.emit for
WorkingMemoryEventType::WorkerSpawned; keep the rest of the chain
(.importance(...).record()) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c5b22b1-aab8-45a5-b101-51132054d3fa
📒 Files selected for processing (41)
docs/design-docs/tiered-memory.mddocs/design-docs/working-memory-example-prompt.mddocs/design-docs/working-memory-implementation-plan.mddocs/design-docs/working-memory-problem-analysis.mddocs/design-docs/working-memory.mdmigrations/20260319000001_working_memory.sqlprompts/en/channel.md.j2prompts/en/compactor.md.j2prompts/en/cortex_daily_summary.md.j2prompts/en/cortex_intraday_synthesis.md.j2prompts/en/cortex_knowledge_synthesis.md.j2prompts/en/memory_persistence.md.j2src/agent/channel.rssrc/agent/channel_dispatch.rssrc/agent/compactor.rssrc/agent/cortex.rssrc/agent/ingestion.rssrc/api/agents.rssrc/api/channels.rssrc/config.rssrc/config/load.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/cron/scheduler.rssrc/lib.rssrc/main.rssrc/memory.rssrc/memory/working.rssrc/prompts/engine.rssrc/prompts/text.rssrc/tools.rssrc/tools/memory_delete.rssrc/tools/memory_persistence_complete.rssrc/tools/memory_save.rssrc/tools/send_agent_message.rssrc/tools/spawn_worker.rssrc/tools/task_create.rssrc/tools/task_update.rstests/bulletin.rstests/context_dump.rs
Comment on lines
+176
to
+180
| {%- if memory_bulletin and not working_memory and not knowledge_synthesis %} | ||
| ## Memory Context | ||
|
|
||
| {{ memory_bulletin }} | ||
| {%- endif %} |
Contributor
There was a problem hiding this comment.
Keep the legacy bulletin until synthesized knowledge exists.
This fallback is gated on both not working_memory and not knowledge_synthesis, so a channel with recent working-memory events but no synthesized knowledge loses its long-term memory context entirely. The fallback should depend only on knowledge_synthesis being absent.
💡 Proposed fix
-{%- if memory_bulletin and not working_memory and not knowledge_synthesis %}
+{%- if memory_bulletin and not knowledge_synthesis %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@prompts/en/channel.md.j2` around lines 176 - 180, The template currently
hides the legacy bulletin when working_memory exists because the condition uses
"memory_bulletin and not working_memory and not knowledge_synthesis"; change
this to only gate on the absence of synthesized knowledge by updating the Jinja
conditional to "memory_bulletin and not knowledge_synthesis" so the bulletin is
shown whenever there's no knowledge_synthesis regardless of working_memory
(refer to the symbols memory_bulletin, working_memory, and knowledge_synthesis
and the conditional block that renders "## Memory Context").
Comment on lines
+16
to
+21
| - Who the agent is, its role, or the company description (Layer 1: Identity) | ||
| - What happened today/yesterday/this week (Layer 2: Working Memory) | ||
| - System status, uptime, memory usage, fleet health (Layer 2: Status Block) | ||
| - Weather, time, or other ephemeral data | ||
| - Product descriptions or GitHub star counts (Layer 1: Identity) | ||
| - "The user is the CEO" or similar role statements (Layer 1: Identity) |
Contributor
There was a problem hiding this comment.
Don't exclude participant-role knowledge until that layer exists.
This rollout does not inject Participant Context yet, so dropping "the user is the CEO"-style facts here leaves stable participant context with nowhere else to live. Keep excluding the agent/company identity, but leave participant-role facts in synthesis until the participant layer ships.
💡 Proposed fix
-- "The user is the CEO" or similar role statements (Layer 1: Identity)
+- The agent's own identity, role, or company description (Layer 1: Identity)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@prompts/en/cortex_knowledge_synthesis.md.j2` around lines 16 - 21, Update the
exclusions in the prompts/en/cortex_knowledge_synthesis.md.j2 template so that
participant-role facts (e.g., the bullet that currently reads '"The user is the
CEO" or similar role statements (Layer 1: Identity)') are NOT excluded from
synthesis until the Participant Context layer exists; keep excluding
agent/company identity entries but remove or reclassify the participant-role
bullet from the exclusion list and add a short note that participant-role facts
should be retained in synthesis until the participant layer ships.
Comment on lines
+587
to
+596
| state | ||
| .deps | ||
| .working_memory | ||
| .emit( | ||
| crate::memory::WorkingMemoryEventType::WorkerSpawned, | ||
| format!("Worker spawned: {task}"), | ||
| ) | ||
| .channel(state.channel_id.to_string()) | ||
| .importance(0.6) | ||
| .record(); |
Contributor
There was a problem hiding this comment.
Don't write raw worker instructions into working memory.
task comes straight from user/channel input. Persisting it verbatim in 30-day working-memory events can capture secrets or PII and re-inject long instructions into later prompts. Emit a scrubbed, truncated summary instead of the full task string.
Suggested fix
+ let worker_summary = {
+ let raw = crate::summarize_first_non_empty_line(task, crate::EVENT_SUMMARY_MAX_CHARS);
+ let scrubbed = match (*secrets_guard).as_ref() {
+ Some(store) => crate::secrets::scrub::scrub_with_store(&raw, store),
+ None => raw,
+ };
+ crate::secrets::scrub::scrub_leaks(&scrubbed)
+ };
+
state
.deps
.working_memory
.emit(
crate::memory::WorkingMemoryEventType::WorkerSpawned,
- format!("Worker spawned: {task}"),
+ format!("Worker spawned: {worker_summary}"),
)
.channel(state.channel_id.to_string())
.importance(0.6)
.record();+ let opencode_summary = {
+ let raw = crate::summarize_first_non_empty_line(task, crate::EVENT_SUMMARY_MAX_CHARS);
+ let scrubbed = match &oc_secrets_store {
+ Some(store) => crate::secrets::scrub::scrub_with_store(&raw, store),
+ None => raw,
+ };
+ crate::secrets::scrub::scrub_leaks(&scrubbed)
+ };
+
state
.deps
.working_memory
.emit(
crate::memory::WorkingMemoryEventType::WorkerSpawned,
- format!("Worker spawned (opencode): {task}"),
+ format!("Worker spawned (opencode): {opencode_summary}"),
)
.channel(state.channel_id.to_string())
.importance(0.6)
.record();Also applies to: 790-799
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/channel_dispatch.rs` around lines 587 - 596, The code currently
writes the raw user-provided task into working memory via
state.deps.working_memory.emit(..., format!("Worker spawned: {task}")) for
WorkingMemoryEventType::WorkerSpawned (and similarly at the other occurrence),
which can persist secrets/PII; change it to emit a scrubbed, truncated summary
instead of the raw task by running the incoming task through a
sanitization/summary helper (e.g., redact_sensitive_and_truncate or
summarize_for_working_memory) before formatting the event string, so the emit
call contains something like "Worker spawned: {scrubbed_summary}" rather than
the verbatim task. Ensure the same fix is applied to the other occurrence
referenced in the review.
Comment on lines
+1955
to
+1958
| // Merges change memory content — bump dirty flag. | ||
| if report.merged > 0 { | ||
| cortex.deps.runtime_config.bump_knowledge_synthesis_version(); | ||
| } |
Contributor
There was a problem hiding this comment.
Mark prunes and decays dirty too.
generate_knowledge_synthesis() depends on both the memory set and importance ordering. A maintenance run that only prunes or decays memories changes that input, but this branch only bumps the version when report.merged > 0, so the cached synthesis can stay stale until some unrelated memory write happens.
Suggested fix
- if report.merged > 0 {
+ if report.merged > 0 || report.pruned > 0 || report.decayed > 0 {
cortex.deps.runtime_config.bump_knowledge_synthesis_version();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Merges change memory content — bump dirty flag. | |
| if report.merged > 0 { | |
| cortex.deps.runtime_config.bump_knowledge_synthesis_version(); | |
| } | |
| // Merges change memory content — bump dirty flag. | |
| if report.merged > 0 || report.pruned > 0 || report.decayed > 0 { | |
| cortex.deps.runtime_config.bump_knowledge_synthesis_version(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/cortex.rs` around lines 1955 - 1958, The version bump only runs
when report.merged > 0, but generate_knowledge_synthesis() also depends on
memory set and importance ordering, so changes from pruning or decay must
trigger the same bump; update the branch in cortex.rs (referencing report.merged
and cortex.deps.runtime_config.bump_knowledge_synthesis_version()) to also call
bump_knowledge_synthesis_version() when report.pruned > 0 or report.decayed > 0
(i.e., bump if any of merged, pruned, or decayed are > 0) so cached synthesis is
invalidated after maintenance runs.
Comment on lines
+467
to
+649
| // ── Render working memory layers (Layers 2 + 3) ── | ||
| let wm_config = **rc.working_memory.load(); | ||
| let wm_timezone = channel_state.deps.working_memory.timezone(); | ||
| let working_memory = crate::memory::working::render_working_memory( | ||
| &channel_state.deps.working_memory, | ||
| &query.channel_id, | ||
| &wm_config, | ||
| wm_timezone, | ||
| ) | ||
| .await | ||
| .unwrap_or_default(); | ||
|
|
||
| let channel_activity_map = crate::memory::working::render_channel_activity_map( | ||
| &channel_state.deps.sqlite_pool, | ||
| &channel_state.deps.working_memory, | ||
| &query.channel_id, | ||
| &wm_config, | ||
| wm_timezone, | ||
| ) | ||
| .await | ||
| .unwrap_or_default(); | ||
|
|
||
| // ── Available channels ── | ||
| let available_channels = { | ||
| let channels = channel_state | ||
| .channel_store | ||
| .list_active() | ||
| .await | ||
| .unwrap_or_default(); | ||
| let entries: Vec<crate::prompts::engine::ChannelEntry> = channels | ||
| .into_iter() | ||
| .filter(|channel| { | ||
| channel.id.as_str() != query.channel_id.as_str() | ||
| && channel.platform != "cron" | ||
| && channel.platform != "webhook" | ||
| }) | ||
| .map(|channel| crate::prompts::engine::ChannelEntry { | ||
| name: channel.display_name.unwrap_or_else(|| channel.id.clone()), | ||
| platform: channel.platform, | ||
| id: channel.id, | ||
| }) | ||
| .collect(); | ||
| if entries.is_empty() { | ||
| None | ||
| } else { | ||
| prompt_engine.render_available_channels(entries).ok() | ||
| } | ||
| }; | ||
|
|
||
| // ── Org context ── | ||
| let org_context = { | ||
| let agent_id = channel_state.deps.agent_id.as_ref(); | ||
| let all_links = channel_state.deps.links.load(); | ||
| let links = crate::links::links_for_agent(&all_links, agent_id); | ||
| if links.is_empty() { | ||
| None | ||
| } else { | ||
| let all_humans = channel_state.deps.humans.load(); | ||
| let humans_by_id: std::collections::HashMap<&str, &crate::config::HumanDef> = | ||
| all_humans.iter().map(|h| (h.id.as_str(), h)).collect(); | ||
|
|
||
| let mut superiors = Vec::new(); | ||
| let mut subordinates = Vec::new(); | ||
| let mut peers = Vec::new(); | ||
|
|
||
| for link in &links { | ||
| let is_from = link.from_agent_id == agent_id; | ||
| let other_id = if is_from { | ||
| &link.to_agent_id | ||
| } else { | ||
| &link.from_agent_id | ||
| }; | ||
| let is_human = humans_by_id.contains_key(other_id.as_str()); | ||
| let (name, role, description) = | ||
| if let Some(human) = humans_by_id.get(other_id.as_str()) { | ||
| let name = human | ||
| .display_name | ||
| .clone() | ||
| .unwrap_or_else(|| other_id.clone()); | ||
| (name, human.role.clone(), human.description.clone()) | ||
| } else { | ||
| let name = channel_state | ||
| .deps | ||
| .agent_names | ||
| .get(other_id.as_str()) | ||
| .cloned() | ||
| .unwrap_or_else(|| other_id.clone()); | ||
| (name, None, None) | ||
| }; | ||
| let info = crate::prompts::engine::LinkedAgent { | ||
| name, | ||
| id: other_id.clone(), | ||
| is_human, | ||
| role, | ||
| description, | ||
| }; | ||
| match link.kind { | ||
| crate::links::LinkKind::Hierarchical => { | ||
| if is_from { | ||
| subordinates.push(info); | ||
| } else { | ||
| superiors.push(info); | ||
| } | ||
| } | ||
| crate::links::LinkKind::Peer => peers.push(info), | ||
| } | ||
| } | ||
|
|
||
| if superiors.is_empty() && subordinates.is_empty() && peers.is_empty() { | ||
| None | ||
| } else { | ||
| prompt_engine | ||
| .render_org_context(crate::prompts::engine::OrgContext { | ||
| superiors, | ||
| subordinates, | ||
| peers, | ||
| }) | ||
| .ok() | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| // ── Adapter prompt ── | ||
| let adapter = query.channel_id.split(':').next().filter(|a| !a.is_empty()); | ||
| let adapter_prompt = | ||
| adapter.and_then(|adapter| prompt_engine.render_channel_adapter_prompt(adapter)); | ||
|
|
||
| // ── Project context ── | ||
| let project_context = { | ||
| use crate::prompts::engine::{ProjectContext, ProjectRepoContext, ProjectWorktreeContext}; | ||
| let store = &channel_state.deps.project_store; | ||
| let projects = store | ||
| .list_projects( | ||
| &channel_state.deps.agent_id, | ||
| Some(crate::projects::ProjectStatus::Active), | ||
| ) | ||
| .await | ||
| .unwrap_or_default(); | ||
| if projects.is_empty() { | ||
| None | ||
| } else { | ||
| let mut contexts = Vec::with_capacity(projects.len()); | ||
| for project in &projects { | ||
| let repos = store.list_repos(&project.id).await.unwrap_or_default(); | ||
| let worktrees = store | ||
| .list_worktrees_with_repos(&project.id) | ||
| .await | ||
| .unwrap_or_default(); | ||
| contexts.push(ProjectContext { | ||
| name: project.name.clone(), | ||
| root_path: project.root_path.clone(), | ||
| description: if project.description.is_empty() { | ||
| None | ||
| } else { | ||
| Some(project.description.clone()) | ||
| }, | ||
| tags: project.tags.clone(), | ||
| repos: repos | ||
| .into_iter() | ||
| .map(|repo| ProjectRepoContext { | ||
| name: repo.name.clone(), | ||
| path: repo.path.clone(), | ||
| default_branch: repo.default_branch.clone(), | ||
| remote_url: if repo.remote_url.is_empty() { | ||
| None | ||
| } else { | ||
| Some(repo.remote_url.clone()) | ||
| }, | ||
| }) | ||
| .collect(), | ||
| worktrees: worktrees | ||
| .into_iter() | ||
| .map(|worktree_with_repo| ProjectWorktreeContext { | ||
| name: worktree_with_repo.worktree.name.clone(), | ||
| path: worktree_with_repo.worktree.path.clone(), | ||
| branch: worktree_with_repo.worktree.branch.clone(), | ||
| repo_name: worktree_with_repo.repo_name.clone(), | ||
| }) | ||
| .collect(), | ||
| }); | ||
| } | ||
| prompt_engine.render_projects_context(contexts).ok() | ||
| } |
Contributor
There was a problem hiding this comment.
Don't hide prompt-assembly failures in inspect_prompt.
These new .unwrap_or_default()/.ok() paths turn database or template errors into empty sections, so the inspect endpoint can return a “valid” prompt while silently omitting exactly the context you're trying to debug. Log and propagate these failures, or surface them explicitly in the response.
💡 Proposed fix pattern
- let working_memory = crate::memory::working::render_working_memory(
+ let working_memory = crate::memory::working::render_working_memory(
&channel_state.deps.working_memory,
&query.channel_id,
&wm_config,
wm_timezone,
)
.await
- .unwrap_or_default();
+ .map_err(|error| {
+ tracing::warn!(
+ %error,
+ channel_id = %query.channel_id,
+ "failed to render working memory for prompt inspect"
+ );
+ StatusCode::INTERNAL_SERVER_ERROR
+ })?;Apply the same handling to render_channel_activity_map, list_active, render_available_channels, render_org_context, and the project-context queries/renders. As per coding guidelines, "Don't silently discard errors. No let _ = on Results. Handle them, log them, or propagate them."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/channels.rs` around lines 467 - 649, The inspect_prompt path is
silently swallowing errors by using unwrap_or_default() and .ok() on calls like
render_working_memory, render_channel_activity_map,
channel_state.channel_store.list_active,
prompt_engine.render_available_channels, prompt_engine.render_org_context,
store.list_projects, store.list_repos, store.list_worktrees_with_repos, and
prompt_engine.render_projects_context; change these to surface failures (return
an Err or a descriptive error response) or at minimum log the error and
propagate it up instead of converting to empty values—replace
unwrap_or_default()/.ok() with proper ? propagation or map_err logging and then
return a failure from inspect_prompt so callers see template/db/render errors
rather than omitted sections.
Comment on lines
+378
to
+386
| exec_context | ||
| .deps | ||
| .working_memory | ||
| .emit( | ||
| crate::memory::WorkingMemoryEventType::Error, | ||
| format!("Cron failed: {exec_job_id}: {error}"), | ||
| ) | ||
| .importance(0.8) | ||
| .record(); |
Contributor
There was a problem hiding this comment.
Avoid persisting raw cron error strings into working memory.
Line 383 stores full error text, which can unintentionally carry sensitive internal details into retained memory/prompt context. Keep detailed errors in logs, and store a redacted summary in working memory instead.
🔒 Suggested fix
exec_context
.deps
.working_memory
.emit(
crate::memory::WorkingMemoryEventType::Error,
- format!("Cron failed: {exec_job_id}: {error}"),
+ format!("Cron failed: {exec_job_id}"),
)
.importance(0.8)
.record();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cron/scheduler.rs` around lines 378 - 386, The code is persisting full
error text into working memory via exec_context.deps.working_memory.emit with
WorkingMemoryEventType::Error using exec_job_id and error; change this to log
the full error (retain existing logs) but only emit a redacted/summary string to
working memory (e.g., "Cron failed: {exec_job_id}: internal error" or a short
sanitized message), ensuring exec_context.deps.working_memory.emit gets the safe
summary and not the raw error payload while keeping the detailed error in
existing logging paths.
Comment on lines
+2548
to
+2559
| // Working memory event log (temporal situational awareness). | ||
| let working_memory_timezone = { | ||
| let user_tz = agent_config.user_timezone.as_deref(); | ||
| let cron_tz = agent_config.cron_timezone.as_deref(); | ||
| user_tz | ||
| .or(cron_tz) | ||
| .and_then(|tz_name| tz_name.parse::<chrono_tz::Tz>().ok()) | ||
| .unwrap_or(chrono_tz::Tz::UTC) | ||
| }; | ||
| let working_memory = | ||
| spacebot::memory::WorkingMemoryStore::new(db.sqlite.clone(), working_memory_timezone); | ||
|
|
Contributor
There was a problem hiding this comment.
Fallback order drops a valid cron_timezone.
This chooses between user_timezone and cron_timezone before parsing. If user_timezone is present but invalid, a valid cron_timezone is never tried and working memory falls back to UTC instead.
Suggested fix
- let working_memory_timezone = {
- let user_tz = agent_config.user_timezone.as_deref();
- let cron_tz = agent_config.cron_timezone.as_deref();
- user_tz
- .or(cron_tz)
- .and_then(|tz_name| tz_name.parse::<chrono_tz::Tz>().ok())
- .unwrap_or(chrono_tz::Tz::UTC)
- };
+ let working_memory_timezone = agent_config
+ .user_timezone
+ .as_deref()
+ .and_then(|tz_name| tz_name.parse::<chrono_tz::Tz>().ok())
+ .or_else(|| {
+ agent_config
+ .cron_timezone
+ .as_deref()
+ .and_then(|tz_name| tz_name.parse::<chrono_tz::Tz>().ok())
+ })
+ .unwrap_or(chrono_tz::Tz::UTC);Also applies to: 2665-2665
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.rs` around lines 2548 - 2559, The timezone fallback currently picks
agent_config.user_timezone.or(agent_config.cron_timezone) before parsing, so an
invalid user_timezone prevents trying a valid cron_timezone; change logic used
to compute working_memory_timezone to parse each option separately: attempt to
parse agent_config.user_timezone (if Some) and if that parse fails, then attempt
to parse agent_config.cron_timezone, and only then fall back to
chrono_tz::Tz::UTC; update the code that constructs working_memory_timezone (the
block feeding WorkingMemoryStore::new) to reflect this sequential-parse fallback
so WorkingMemoryStore::new still receives a valid chrono_tz::Tz.
Comment on lines
+236
to
+246
| if let Some(working_memory) = &self.working_memory { | ||
| working_memory | ||
| .emit( | ||
| crate::memory::WorkingMemoryEventType::TaskUpdate, | ||
| format!( | ||
| "Task #{} updated to {}", | ||
| updated.task_number, updated.status | ||
| ), | ||
| ) | ||
| .importance(0.4) | ||
| .record(); |
Contributor
There was a problem hiding this comment.
Emit what actually changed, not always the current status.
Lines 240-243 record Task #N updated to <status> for every successful update. Title/description edits, subtask completions, metadata merges, approvals, or even no-op updates will all be written into working memory as a status change, which pollutes the recent-context layer injected into channel prompts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/task_update.rs` around lines 236 - 246, The current
working_memory.emit call always logs "Task #{} updated to {}" using
updated.status, which misrepresents edits; update the code around
working_memory.emit (the emit call for
crate::memory::WorkingMemoryEventType::TaskUpdate) to compute the actual delta
between the previous and new task (compare fields like title, description,
status, subtasks, approvals, metadata, etc.), then emit a concise message
describing what changed (e.g., "Task `#N` title changed", "Task `#N` description
edited", "Task `#N` subtask X completed", "Task `#N` metadata merged", or "Task `#N`
no-op") or use more specific event types if available; reference
updated.task_number and the prior task/state to build the message so the working
memory records the real change instead of always writing the status.
Knowledge synthesis is stable long-term context (decisions, goals, preferences) that changes a few times a day. Working memory is what's happening right now. Stable context should load before temporal context — the LLM reads the knowledge foundation before the live event stream.
R1: borrow ProcessEvent in matches! to avoid move before signal_from_event R11: include unsynthesized tail events in daily summary instead of dropping them R15: use floor_char_boundary for safe UTF-8 truncation in channel activity map
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/agent/cortex.rs (7)
1955-1958:⚠️ Potential issue | 🟠 MajorPrunes and decays should also trigger synthesis invalidation.
This condition only bumps the dirty flag when memories are merged, but
generate_knowledge_synthesis()also depends on pruned/decayed memories. Maintenance runs that only prune or decay will leave the cached synthesis stale.Suggested fix
- if report.merged > 0 { + if report.merged > 0 || report.pruned > 0 || report.decayed > 0 { cortex.deps.runtime_config.bump_knowledge_synthesis_version(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex.rs` around lines 1955 - 1958, The dirty-flag bump currently only runs when report.merged > 0, but generate_knowledge_synthesis() also depends on pruning and decay; update the condition around cortex.deps.runtime_config.bump_knowledge_synthesis_version() to also run when report.pruned > 0 or report.decayed > 0 (i.e., replace the single merged check with a combined check like merged || pruned || decayed) so maintenance runs that only prune or decay will invalidate the cached synthesis.
2576-2579: 🛠️ Refactor suggestion | 🟠 MajorAdd explicit
default_max_turns(1)to this agent builder.Per coding guidelines, Rig defaults
max_turnsto 0 and should always be set explicitly. This is a one-shot.prompt()call, so it should use.default_max_turns(1).Suggested fix
let agent = AgentBuilder::new(model) .preamble(&synthesis_preamble) .hook(CortexHook::new()) + .default_max_turns(1) .build();As per coding guidelines: "Rig defaults max_turns to 0 (single call). Always set explicitly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex.rs` around lines 2576 - 2579, The AgentBuilder created for `agent` currently doesn't set an explicit max turn and relies on defaults; update the builder chain that constructs `AgentBuilder::new(model)` (the call that then uses `.preamble(&synthesis_preamble).hook(CortexHook::new()).build()`) to include `.default_max_turns(1)` before `.build()` so the agent is explicitly configured for a single-shot `.prompt()` call.
2094-2106:⚠️ Potential issue | 🟠 MajorGuard knowledge synthesis refresh with the warmup lock.
The warmup path acquires
warmup_lockbefore callinggenerate_knowledge_synthesis(), but this dirty-flag refresh path spawns it directly without the lock. This allows two syntheses to race and write conflicting values toknowledge_synthesis,memory_bulletin, and the version trackers.Suggested fix
if refresh_task.is_none() && should_regenerate_knowledge_synthesis(&cortex.deps) { let deps = cortex.deps.clone(); let synthesis_logger = logger.clone(); + let warmup_lock = deps.runtime_config.warmup_lock.clone(); refresh_task = Some(tokio::spawn(async move { + let _guard = warmup_lock.lock().await; let success = generate_knowledge_synthesis(&deps, &synthesis_logger).await; if success { BulletinRefreshOutcome::Generated } else { BulletinRefreshOutcome::Failed } })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex.rs` around lines 2094 - 2106, The refresh spawn path currently calls generate_knowledge_synthesis without holding the same warmup_lock used by the warmup flow, allowing races on knowledge_synthesis/memory_bulletin and version state; modify the branch that sets refresh_task (the block that checks refresh_task.is_none() && should_regenerate_knowledge_synthesis(&cortex.deps)) to acquire a clone of the warmup_lock (or capture it) and hold it inside the spawned async task around the call to generate_knowledge_synthesis so the task obtains the same lock (warmup_lock) before invoking generate_knowledge_synthesis(&deps, &synthesis_logger), ensuring only one synthesis runs and updates knowledge_synthesis, memory_bulletin, and version trackers at a time.
2539-2548:⚠️ Potential issue | 🟠 MajorEmpty result doesn't clear the dirty flag, causing infinite retries.
When
raw_sections.is_empty(), the function returnstrue(success) but never updatesknowledge_synthesis_last_version(that happens at lines 2608-2614 after this early return). The version mismatch persists, soshould_regenerate_knowledge_synthesis()keeps returning true and the cortex reschedules synthesis after every debounce window.Suggested fix
if raw_sections.is_empty() { tracing::info!("no memories found for knowledge synthesis"); deps.runtime_config .knowledge_synthesis .store(Arc::new(String::new())); // Keep bulletin in sync during transition. deps.runtime_config .memory_bulletin .store(Arc::new(String::new())); + // Mark this version as synthesized to clear the dirty flag. + let current = deps + .runtime_config + .knowledge_synthesis_version + .load(std::sync::atomic::Ordering::Relaxed); + deps.runtime_config + .knowledge_synthesis_last_version + .store(current, std::sync::atomic::Ordering::Relaxed); return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex.rs` around lines 2539 - 2548, When raw_sections.is_empty() the function currently returns true but doesn't update the knowledge_synthesis_last_version, so should_regenerate_knowledge_synthesis() stays true and causes infinite retries; modify the early-empty branch in the function containing raw_sections handling to also update deps.runtime_config.knowledge_synthesis_last_version (the same place where it's updated later around knowledge_synthesis_last_version handling) to the current version/timestamp and then return true, and ensure you still store empty strings into knowledge_synthesis and memory_bulletin as before so the dirty flag is cleared when there are no sections.
2826-2829: 🛠️ Refactor suggestion | 🟠 MajorAdd explicit
default_max_turns(1)and move the inline preamble to a prompt file.This builder has two guideline violations:
- Missing
.default_max_turns(1)for a one-shot call- Hardcoded preamble string instead of loading from
prompts/Suggested fix
+ let intraday_preamble = match prompt_engine.render_static("cortex_intraday_synthesis") { + Ok(p) => p, + Err(error) => { + return Err(anyhow::anyhow!("failed to render cortex_intraday_synthesis: {error}")); + } + }; + let agent = AgentBuilder::new(model) - .preamble("You are a concise narrative summarizer. Output only the summary paragraph, nothing else.") + .preamble(&intraday_preamble) .hook(CortexHook::new()) + .default_max_turns(1) .build();As per coding guidelines: "Don't store prompts as string constants in Rust" and "Rig defaults max_turns to 0. Always set explicitly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex.rs` around lines 2826 - 2829, Update the AgentBuilder usage to explicitly set default_max_turns to 1 and stop inlining the preamble string: call .default_max_turns(1) on the AgentBuilder chain (alongside .hook(CortexHook::new()) and .build()) and replace the hardcoded .preamble(...) with loading the prompt from the prompts directory (e.g., read or include the prompt file from prompts/ and pass that string to .preamble). Keep references to AgentBuilder, .default_max_turns(1), .preamble(...), CortexHook::new(), and .build() so the change is applied to the same builder chain.
2158-2166:⚠️ Potential issue | 🟠 MajorAvoid blocking the main loop with inline LLM calls.
maybe_synthesize_intraday_batch()andmaybe_synthesize_daily_summary()can make full LLM calls (lines 2831, 2973), but they're awaited inline inside the tick handler. While synthesis runs,event_rxandmemory_event_rxstop being drained, causing the cortex to lag or drop events exactly when activity is highest.Suggested fix: spawn as background tasks
// Working memory: intra-day synthesis (cheap SQL check, LLM only on threshold). - if let Err(error) = maybe_synthesize_intraday_batch(&cortex.deps, logger).await { - tracing::warn!(%error, "intra-day synthesis check failed"); - } - - // Working memory: daily summary for yesterday (idempotent, 1 LLM call/day max). - if let Err(error) = maybe_synthesize_daily_summary(&cortex.deps, logger).await { - tracing::warn!(%error, "daily summary check failed"); - } + { + let deps = cortex.deps.clone(); + let intraday_logger = logger.clone(); + tokio::spawn(async move { + if let Err(error) = maybe_synthesize_intraday_batch(&deps, &intraday_logger).await { + tracing::warn!(%error, "intra-day synthesis check failed"); + } + }); + } + { + let deps = cortex.deps.clone(); + let daily_logger = logger.clone(); + tokio::spawn(async move { + if let Err(error) = maybe_synthesize_daily_summary(&deps, &daily_logger).await { + tracing::warn!(%error, "daily summary check failed"); + } + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex.rs` around lines 2158 - 2166, The inline awaits of maybe_synthesize_intraday_batch(&cortex.deps, logger).await and maybe_synthesize_daily_summary(&cortex.deps, logger).await block the tick handler and prevent draining event_rx/memory_event_rx; instead spawn each synthesis call as a background task (e.g., tokio::spawn) so they run concurrently without awaiting in the main loop, clone or Arc-wrap cortex.deps and clone/derive a logger for the task closure, and inside the spawned async move handle the Result exactly as before (logging any Err with tracing::warn) to preserve behavior while avoiding blocking the event loop.
2607-2614:⚠️ Potential issue | 🟠 MajorSnapshot the version before async work, not after.
The code reads
knowledge_synthesis_versionafter the LLM call completes (line 2608-2611). If a memory write lands duringgather_sections_from_list()oragent.prompt(), the version gets bumped, but this code then stores that newer version as "synthesized" — even though the output was based on older data. This suppresses the necessary follow-up regeneration.Suggested fix: snapshot before async work
pub async fn generate_knowledge_synthesis(deps: &AgentDeps, logger: &CortexLogger) -> bool { tracing::info!("cortex generating knowledge synthesis"); let started = Instant::now(); + + // Snapshot the version we're synthesizing against BEFORE async work. + let target_version = deps + .runtime_config + .knowledge_synthesis_version + .load(std::sync::atomic::Ordering::Relaxed); // Gather narrower sections (no identity, no events, no recent). let raw_sections = gather_sections_from_list(deps, KNOWLEDGE_SYNTHESIS_SECTIONS).await;Then later, store
target_versioninstead of re-reading:// Mark this version as synthesized. - let current = deps - .runtime_config - .knowledge_synthesis_version - .load(std::sync::atomic::Ordering::Relaxed); deps.runtime_config .knowledge_synthesis_last_version - .store(current, std::sync::atomic::Ordering::Relaxed); + .store(target_version, std::sync::atomic::Ordering::Relaxed);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex.rs` around lines 2607 - 2614, Snapshot the current knowledge_synthesis_version before performing any async work (e.g., before calling gather_sections_from_list() or agent.prompt()), store that snapshot in a local variable like target_version, and after the LLM call complete write that saved target_version into runtime_config. In other words, stop re-reading runtime_config.knowledge_synthesis_version after the async calls and instead store the pre-call snapshot into runtime_config.knowledge_synthesis_last_version so the recorded "synthesized" version matches the data used to generate the output.
🧹 Nitpick comments (1)
src/agent/cortex.rs (1)
2657-2707: Consider extracting shared logic withgather_bulletin_sections.This function is nearly identical to
gather_bulletin_sections(lines 2260-2314). The only difference is the section list passed in. Consider refactoringgather_bulletin_sectionsto call this helper to reduce duplication.Suggested refactor
async fn gather_bulletin_sections(deps: &AgentDeps) -> String { - let mut output = String::new(); - - for section in BULLETIN_SECTIONS { - // ... duplicated logic ... - } - + let mut output = gather_sections_from_list(deps, BULLETIN_SECTIONS).await; + // Append active tasks (non-done) from the task store. match gather_active_tasks(deps).await { Ok(section) if !section.is_empty() => output.push_str(§ion), // ... } output }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex.rs` around lines 2657 - 2707, The logic in gather_sections_from_list duplicates gather_bulletin_sections; refactor by removing the duplicated body in gather_bulletin_sections and have it delegate to gather_sections_from_list(deps, §ions) (preserving types AgentDeps and &[BulletinSection]), ensuring the same SearchConfig/result formatting is used and error handling/tracing remains unchanged; update any local variable names and imports if needed and delete the now-redundant duplicate code block so gather_sections_from_list is the single implementation of the shared logic.
🤖 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/cortex.rs`:
- Around line 2968-2971: The AgentBuilder usage must set an explicit turn limit
and remove the hardcoded preamble string: update the chain that constructs
AgentBuilder::new(model) to call .default_max_turns(1) and replace
.preamble("You are a daily activity summarizer. Output only the summary, nothing
else.") with a preamble loaded from an external prompt file (e.g., use a file
include or loader and pass that string into .preamble), keeping the
.hook(CortexHook::new()) and .build() calls unchanged; ensure the prompt content
is extracted to a prompt file per guidelines and referenced by the AgentBuilder
preamble call.
---
Duplicate comments:
In `@src/agent/cortex.rs`:
- Around line 1955-1958: The dirty-flag bump currently only runs when
report.merged > 0, but generate_knowledge_synthesis() also depends on pruning
and decay; update the condition around
cortex.deps.runtime_config.bump_knowledge_synthesis_version() to also run when
report.pruned > 0 or report.decayed > 0 (i.e., replace the single merged check
with a combined check like merged || pruned || decayed) so maintenance runs that
only prune or decay will invalidate the cached synthesis.
- Around line 2576-2579: The AgentBuilder created for `agent` currently doesn't
set an explicit max turn and relies on defaults; update the builder chain that
constructs `AgentBuilder::new(model)` (the call that then uses
`.preamble(&synthesis_preamble).hook(CortexHook::new()).build()`) to include
`.default_max_turns(1)` before `.build()` so the agent is explicitly configured
for a single-shot `.prompt()` call.
- Around line 2094-2106: The refresh spawn path currently calls
generate_knowledge_synthesis without holding the same warmup_lock used by the
warmup flow, allowing races on knowledge_synthesis/memory_bulletin and version
state; modify the branch that sets refresh_task (the block that checks
refresh_task.is_none() && should_regenerate_knowledge_synthesis(&cortex.deps))
to acquire a clone of the warmup_lock (or capture it) and hold it inside the
spawned async task around the call to generate_knowledge_synthesis so the task
obtains the same lock (warmup_lock) before invoking
generate_knowledge_synthesis(&deps, &synthesis_logger), ensuring only one
synthesis runs and updates knowledge_synthesis, memory_bulletin, and version
trackers at a time.
- Around line 2539-2548: When raw_sections.is_empty() the function currently
returns true but doesn't update the knowledge_synthesis_last_version, so
should_regenerate_knowledge_synthesis() stays true and causes infinite retries;
modify the early-empty branch in the function containing raw_sections handling
to also update deps.runtime_config.knowledge_synthesis_last_version (the same
place where it's updated later around knowledge_synthesis_last_version handling)
to the current version/timestamp and then return true, and ensure you still
store empty strings into knowledge_synthesis and memory_bulletin as before so
the dirty flag is cleared when there are no sections.
- Around line 2826-2829: Update the AgentBuilder usage to explicitly set
default_max_turns to 1 and stop inlining the preamble string: call
.default_max_turns(1) on the AgentBuilder chain (alongside
.hook(CortexHook::new()) and .build()) and replace the hardcoded .preamble(...)
with loading the prompt from the prompts directory (e.g., read or include the
prompt file from prompts/ and pass that string to .preamble). Keep references to
AgentBuilder, .default_max_turns(1), .preamble(...), CortexHook::new(), and
.build() so the change is applied to the same builder chain.
- Around line 2158-2166: The inline awaits of
maybe_synthesize_intraday_batch(&cortex.deps, logger).await and
maybe_synthesize_daily_summary(&cortex.deps, logger).await block the tick
handler and prevent draining event_rx/memory_event_rx; instead spawn each
synthesis call as a background task (e.g., tokio::spawn) so they run
concurrently without awaiting in the main loop, clone or Arc-wrap cortex.deps
and clone/derive a logger for the task closure, and inside the spawned async
move handle the Result exactly as before (logging any Err with tracing::warn) to
preserve behavior while avoiding blocking the event loop.
- Around line 2607-2614: Snapshot the current knowledge_synthesis_version before
performing any async work (e.g., before calling gather_sections_from_list() or
agent.prompt()), store that snapshot in a local variable like target_version,
and after the LLM call complete write that saved target_version into
runtime_config. In other words, stop re-reading
runtime_config.knowledge_synthesis_version after the async calls and instead
store the pre-call snapshot into runtime_config.knowledge_synthesis_last_version
so the recorded "synthesized" version matches the data used to generate the
output.
---
Nitpick comments:
In `@src/agent/cortex.rs`:
- Around line 2657-2707: The logic in gather_sections_from_list duplicates
gather_bulletin_sections; refactor by removing the duplicated body in
gather_bulletin_sections and have it delegate to gather_sections_from_list(deps,
§ions) (preserving types AgentDeps and &[BulletinSection]), ensuring the
same SearchConfig/result formatting is used and error handling/tracing remains
unchanged; update any local variable names and imports if needed and delete the
now-redundant duplicate code block so gather_sections_from_list is the single
implementation of the shared logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8515932f-e37f-4733-bd02-92a7450bed54
📒 Files selected for processing (3)
docs/design-docs/working-memory-triage.mdsrc/agent/cortex.rssrc/memory/working.rs
✅ Files skipped from review due to trivial changes (2)
- docs/design-docs/working-memory-triage.md
- src/memory/working.rs
Comment on lines
+2968
to
+2971
| let agent = AgentBuilder::new(model) | ||
| .preamble("You are a daily activity summarizer. Output only the summary, nothing else.") | ||
| .hook(CortexHook::new()) | ||
| .build(); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit default_max_turns(1) and move the inline preamble to a prompt file.
Same issues as the intraday synthesis builder — missing explicit max_turns and hardcoded preamble.
Suggested fix
+ let daily_preamble = match prompt_engine.render_static("cortex_daily_summary") {
+ Ok(p) => p,
+ Err(error) => {
+ return Err(anyhow::anyhow!("failed to render cortex_daily_summary: {error}"));
+ }
+ };
+
let agent = AgentBuilder::new(model)
- .preamble("You are a daily activity summarizer. Output only the summary, nothing else.")
+ .preamble(&daily_preamble)
.hook(CortexHook::new())
+ .default_max_turns(1)
.build();As per coding guidelines: "Don't store prompts as string constants in Rust" and "Rig defaults max_turns to 0. Always set explicitly."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let agent = AgentBuilder::new(model) | |
| .preamble("You are a daily activity summarizer. Output only the summary, nothing else.") | |
| .hook(CortexHook::new()) | |
| .build(); | |
| let daily_preamble = match prompt_engine.render_static("cortex_daily_summary") { | |
| Ok(p) => p, | |
| Err(error) => { | |
| return Err(anyhow::anyhow!("failed to render cortex_daily_summary: {error}")); | |
| } | |
| }; | |
| let agent = AgentBuilder::new(model) | |
| .preamble(&daily_preamble) | |
| .hook(CortexHook::new()) | |
| .default_max_turns(1) | |
| .build(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/cortex.rs` around lines 2968 - 2971, The AgentBuilder usage must
set an explicit turn limit and remove the hardcoded preamble string: update the
chain that constructs AgentBuilder::new(model) to call .default_max_turns(1) and
replace .preamble("You are a daily activity summarizer. Output only the summary,
nothing else.") with a preamble loaded from an external prompt file (e.g., use a
file include or loader and pass that string into .preamble), keeping the
.hook(CortexHook::new()) and .build() calls unchanged; ensure the prompt content
is extracted to a prompt file per guidelines and referenced by the AgentBuilder
preamble call.
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
1 participant
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
Replaces the timer-based bulletin system with a 5-layer context assembly architecture that gives channels temporal awareness, cross-channel visibility, and change-driven knowledge synthesis. The old bulletin regenerated 24 times/day (every 60 min) regardless of changes, was identical for all channels, and cost ~240k tokens/day per agent. The new system scales with actual activity, not wall-clock time.
Problem
Users churn because the agent feels "stupid and forgetful":
docs/design-docs/working-memory-problem-analysis.mdfor the full 10-failure-mode catalogArchitecture
Five layers injected into every channel system prompt, each with a distinct data source and refresh cadence:
working_memory_eventstablechannels+conversation_messagesmemoriestable via cortex LLMPrompt ordering (reordered in this PR)
Static instructions load first so the LLM has its operating manual before dynamic context. Dynamic layers sit closest to the conversation history where recency bias in attention matters most.
Implementation Phases
Phase 1: Event Store + Emission
working_memory_events,working_memory_intraday_syntheses,working_memory_daily_summariestablesWorkingMemoryStorewith fire-and-forget recording, timezone-aware day computation, builder APIPhase 2: Context Injection
render_working_memory()— today's intra-day syntheses + unsynthesized event tail + yesterday + this week, with token budget (60/20/20 split)render_channel_activity_map()— single SQL query with correlated subquery, topic hints via window function, time-ago formattingworking_memory,channel_activity_map,knowledge_synthesisWorkingMemoryConfig(15 fields with defaults) onRuntimeConfigPhase 3: Knowledge Synthesis (Replace Bulletin)
knowledge_synthesis_version(AtomicU64) bumped on memory content changes (create/update/delete/merge), NOT on importance-only changesgenerate_knowledge_synthesis()— 5 narrower sections (Decisions, High-Importance, Preferences, Goals, Observations) vs bulletin's 8cortex_knowledge_synthesis.md.j2prompt with strong exclusion rules (no identity, no events, no status)BulletinStaleno longer blocks work readinessPhase 4: Intra-Day Synthesis + Daily Summaries
maybe_synthesize_intraday_batch()— dual trigger (≥15 events OR 4-hour time fallback), one-shot LLM call produces 50-100 word narrative paragraphmaybe_synthesize_daily_summary()— idempotent (1 call/day max), uses already-digested intra-day paragraphs as inputPhase 5a: Smarter Persistence Triggers
MemoryPersistenceCompleteArgs.eventsfield for extracted decisions/errors/system eventsBranchToolProfile::MemoryPersistencePhase 5b: Sunset Compactor Memory Extraction
memory_save— now a pure single-shot summarizermax_turnsreduced from 10 to 1create_cortex_tool_servermarked dead codeAdditional Fixes
/channels/inspect): Previously passedNonefor working_memory, channel_activity_map, available_channels, org_context, adapter_prompt, and project_context — the snapshot showed a different prompt than what the LLM actually saw. Now renders all dynamic sections to match the real prompt exactly.Files Changed (41 files, +6287 -93)
New files
src/memory/working.rsmigrations/20260319000001_working_memory.sqlprompts/en/cortex_knowledge_synthesis.md.j2prompts/en/cortex_intraday_synthesis.md.j2prompts/en/cortex_daily_summary.md.j2docs/design-docs/working-memory*.mddocs/design-docs/tiered-memory.mdModified files (key changes)
src/agent/cortex.rssrc/agent/channel.rssrc/api/channels.rssrc/config/types.rsWorkingMemoryConfig, cortex config fieldssrc/config/runtime.rsprompts/en/channel.md.j2prompts/en/compactor.md.j2Testing
just gate-prgreen (fmt, clippy, check, test, migration safety)Cost Impact