feat(memory): add team-level memory attached to the team object#3972
Open
jbecke wants to merge 5 commits into
Open
feat(memory): add team-level memory attached to the team object#3972jbecke wants to merge 5 commits into
jbecke wants to merge 5 commits into
Conversation
Mirrors the personal memory system for teams: - team_memory table (one row per team, FK to team, upsert on team_id) - TeamMemoryRepo / TeamMemoryService ports + PgTeamMemoryRepo - TeamMemoryServiceImpl with team-focused generation and judge prompts, 24h freshness window, and a Postgres advisory lock so concurrent members don't trigger duplicate generations - GET /memory/team endpoint on the document cognition service - <team_memory> injected into chat and scheduled-agent system prompts alongside <user_memory> https://claude.ai/code/session_01CyfX47q3gqFp49pLrcco8b
Contributor
📝 WalkthroughWalkthroughThis PR adds team memory support to the memory service system, enabling agents to maintain and retrieve contextual memory at both the user and team level. The implementation extends the database schema to store team-scoped memory records, introduces domain types to represent dual-memory results, implements a team memory generation pipeline with concurrent retrieval and cross-instance locking, updates the HTTP API to expose both memories, and integrates team memory into chat, document cognition, and scheduled agent endpoints. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Restores the .sqlx entries accidentally deleted in the previous commit (cargo sqlx prepare clears the directory while it runs) and adds the new team_memory query metadata. https://claude.ai/code/session_01CyfX47q3gqFp49pLrcco8b
Contributor
|
Looks super close. Can we create one memory service that we can call Also looks like this is last write wins. Probably not a problem |
…ervice Review feedback from #3972: - team memory is now a team_id-scoped row in the existing memory table (user_id/team_id are mutually exclusive, partial unique indexes keep one-row-per-owner upsert semantics) instead of a separate table - one MemoryService::get_or_generate_memory call now fetches both the personal and team memory and triggers both background generations; the separate TeamMemoryService and GET /memory/team endpoint are gone - GET /memory returns { memory?, team_memory? }; regenerated OpenAPI spec and TypeScript client https://claude.ai/code/session_01CyfX47q3gqFp49pLrcco8b
Contributor
Author
|
Both addressed in 0407542:
On last-write-wins: agreed it's fine for personal memory, but since any member of a team can trigger a refresh, concurrent team generations are serialized with a Generated by Claude Code |
![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: 4
🧹 Nitpick comments (3)
rust/cloud-storage/memory/src/domain/ports.rs (1)
78-103: ⚡ Quick winAdd documentation to the
TeamMemoryRepotrait.The
TeamMemoryRepotrait is missing a doc comment. If this crate has#![deny(missing_docs)]enabled (per Rust guidelines for new crates), this will cause a compilation error.📝 Suggested doc comment
+/// Repository for team-scoped memory persistence. +/// +/// Team memory is stored in the same table as user memory but scoped to +/// `team_id` rather than `user_id`. pub trait TeamMemoryRepo: Send + Sync + 'static {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/cloud-storage/memory/src/domain/ports.rs` around lines 78 - 103, Add a doc comment for the TeamMemoryRepo trait (place a /// comment block immediately above pub trait TeamMemoryRepo) describing its purpose as the in-memory persistence interface for team-related memories and lookups, and briefly document each provided method (save_team_memory, get_latest_team_memory, get_team_memory_by_id, get_user_team_id, get_team_overview) so users know what each does and expected return semantics; ensure the comments are descriptive enough to satisfy `#![deny(missing_docs)]` and mention any important behaviors (e.g., Option returns, errors wrapped in Result, async Future returns).Source: Coding guidelines
rust/cloud-storage/memory/src/outbound/pg_team_memory_repo.rs (1)
12-20: ⚡ Quick winAdd documentation to public items.
The
PgTeamMemoryRepostruct and itsnewconstructor lack doc comments. If this crate has#![deny(missing_docs)]enabled, this will cause a compilation error.📝 Suggested doc comments
+/// Postgres-backed repository for team memory storage. pub struct PgTeamMemoryRepo { inner: PgPool, } impl PgTeamMemoryRepo { + /// Create a new team memory repository with the given database pool. pub fn new(inner: PgPool) -> Self {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/cloud-storage/memory/src/outbound/pg_team_memory_repo.rs` around lines 12 - 20, Add missing documentation comments for the public struct PgTeamMemoryRepo and its constructor method new: add a brief /// doc comment above the PgTeamMemoryRepo declaration describing its purpose (e.g., in-memory/outbound team repository backed by a PgPool) and a /// doc comment above impl PgTeamMemoryRepo::new that explains the parameter and what the constructor returns. Ensure the comments follow crate style (short summary sentence, optional longer description) so the crate compiles with #![deny(missing_docs)].Source: Coding guidelines
rust/cloud-storage/macro_db_client/migrations/20260610212946_add_team_id_to_memory_table.sql (1)
3-11: ⚖️ Poor tradeoffConsider adding idempotency guards to the migration.
The migration uses
ALTER TABLEstatements that will fail if re-run. While SQLx tracks applied migrations and won't re-execute them, the SQL review guidelines recommend making migrations idempotent where possible.You could wrap the alterations in a DO block that checks for column existence, though this adds complexity for minimal practical benefit given SQLx's migration tracking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/cloud-storage/macro_db_client/migrations/20260610212946_add_team_id_to_memory_table.sql` around lines 3 - 11, Make the migration idempotent by guarding each schema change for the memory table: check that the user_id column exists and currently has a NOT NULL constraint before running ALTER COLUMN to DROP NOT NULL; add team_id using ADD COLUMN IF NOT EXISTS (with the REFERENCES team(id) ON DELETE CASCADE) or a conditional check for the column in pg_catalog/information_schema; add the memory_user_or_team CHECK only if it doesn't already exist (check pg_constraint by conname); use DROP CONSTRAINT IF EXISTS memory_user_id_unique before attempting to drop it; and create the two partial unique indexes (memory_user_id_unique and memory_team_id_unique) using CREATE UNIQUE INDEX IF NOT EXISTS (or wrap creation in a conditional that checks pg_index/pg_class) so re-running the migration is safe.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/cloud-storage/memory/src/domain/service.rs`:
- Around line 107-119: In get_or_generate_memory, avoid treating the joined
results from tokio::join! as all-or-nothing: instead of immediately using the
`?` on both results, handle each Result returned by get_or_generate_user_memory
and get_or_generate_team_memory individually so one Err does not discard the
other's Ok; call both functions with tokio::join!, map each result to either a
successful memory or None / a fallback value, log or propagate errors
selectively if needed, and construct Memories { user: ..., team: ... } using the
available values so the endpoint can return whichever scope(s) exist.
In `@rust/cloud-storage/memory/src/domain/service/team.rs`:
- Around line 128-130: The code calls svc.generate_team_memory(team_id,
user.clone(), previous_memory) using the triggering user's permissions, which
risks leaking artifacts private to that user into a shared team memory; update
the call and implementation to use a team-scoped visibility check instead of a
single user's view by (a) changing generate_team_memory to accept the team's
membership list or a team-permission context (e.g., team_id plus Vec<member_ids>
or a TeamPermission object) and (b) inside generate_team_memory only include
artifacts visible to every teammate (compute intersection of visible artifact
IDs) or otherwise restrict to artifacts explicitly marked as team-visible;
update every call site (the current svc.generate_team_memory invocations and
analogous calls at the other noted locations) to pass the team
membership/permission context so the persisted team memory contains only data
safe for all members.
- Around line 99-140: The spawned task captures needs_generation and
previous_memory before acquiring the advisory lock, allowing a stale generation
to proceed; after acquiring the lock via try_acquire_generation_lock(&pool,
team_id).await but before calling svc.generate_team_memory(...), re-fetch the
current row/state (the same source used to compute
needs_generation/previous_memory) and re-check whether generation is still
required and whether previous_memory changed, and only call
MemoryServiceImpl::generate_team_memory(team_id, user.clone(), previous_memory)
when the re-check indicates generation is still needed; if not needed, log and
return. Ensure this re-check uses the same repo/db pool and the same semantics
as the original needs_generation computation so the advisory lock enforces
deduplication rather than just serialization.
In
`@rust/cloud-storage/scheduled_action/src/outbound/inprocess_executor/agent_task.rs`:
- Around line 125-131: The None branch for building system_prompt drops
SCHEDULED_AGENT_PROMPT so scheduled runs lose guardrails; update the None branch
that currently returns tools.prompt.to_string() to include
SCHEDULED_AGENT_PROMPT (e.g., concatenate tools.prompt and
SCHEDULED_AGENT_PROMPT with a newline) so system_prompt always contains both
tools.prompt and SCHEDULED_AGENT_PROMPT even when memories.user is None; modify
the code around system_prompt, memories.user, SCHEDULED_AGENT_PROMPT and
tools.prompt accordingly.
---
Nitpick comments:
In
`@rust/cloud-storage/macro_db_client/migrations/20260610212946_add_team_id_to_memory_table.sql`:
- Around line 3-11: Make the migration idempotent by guarding each schema change
for the memory table: check that the user_id column exists and currently has a
NOT NULL constraint before running ALTER COLUMN to DROP NOT NULL; add team_id
using ADD COLUMN IF NOT EXISTS (with the REFERENCES team(id) ON DELETE CASCADE)
or a conditional check for the column in pg_catalog/information_schema; add the
memory_user_or_team CHECK only if it doesn't already exist (check pg_constraint
by conname); use DROP CONSTRAINT IF EXISTS memory_user_id_unique before
attempting to drop it; and create the two partial unique indexes
(memory_user_id_unique and memory_team_id_unique) using CREATE UNIQUE INDEX IF
NOT EXISTS (or wrap creation in a conditional that checks pg_index/pg_class) so
re-running the migration is safe.
In `@rust/cloud-storage/memory/src/domain/ports.rs`:
- Around line 78-103: Add a doc comment for the TeamMemoryRepo trait (place a
/// comment block immediately above pub trait TeamMemoryRepo) describing its
purpose as the in-memory persistence interface for team-related memories and
lookups, and briefly document each provided method (save_team_memory,
get_latest_team_memory, get_team_memory_by_id, get_user_team_id,
get_team_overview) so users know what each does and expected return semantics;
ensure the comments are descriptive enough to satisfy `#![deny(missing_docs)]`
and mention any important behaviors (e.g., Option returns, errors wrapped in
Result, async Future returns).
In `@rust/cloud-storage/memory/src/outbound/pg_team_memory_repo.rs`:
- Around line 12-20: Add missing documentation comments for the public struct
PgTeamMemoryRepo and its constructor method new: add a brief /// doc comment
above the PgTeamMemoryRepo declaration describing its purpose (e.g.,
in-memory/outbound team repository backed by a PgPool) and a /// doc comment
above impl PgTeamMemoryRepo::new that explains the parameter and what the
constructor returns. Ensure the comments follow crate style (short summary
sentence, optional longer description) so the crate compiles with
#![deny(missing_docs)].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a828049f-bba2-4aca-ba25-acabfc5e766e
⛔ Files ignored due to path filters (11)
js/app/packages/service-clients/service-cognition/generated/client.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/index.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/memoryResponse.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/memoryResponseMemory.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/schemas/memoryResponseTeamMemory.tsis excluded by!**/generated/**rust/cloud-storage/.sqlx/query-2bacd87def214ac03a40e3b5858a67bc5e7f96ca1d145e65f0c6e3f2787cf227.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-5a427312ded69610ede4679d54cefaf14c6da39d8e7a2ade3b370343002be82c.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-91bfe5ec5b5f8eadaa5a149987c59081daf7cd5fb66aa4d8299f39c96e402d80.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-bdbe32af4a2c3138e6853ec1d541abbce728b93591e34fb09d9ce8bfa19729c1.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-d932a832244139cad77a3dcfb0b11ff406ea348014419a970a7a4ea5b7a455b0.jsonis excluded by!**/.sqlx/**rust/cloud-storage/.sqlx/query-ea3b56b003d40d52eb5fe2e247fa38e7cfbcff3def9db49e5d9bad83207a3a48.jsonis excluded by!**/.sqlx/**
📒 Files selected for processing (18)
js/app/packages/service-clients/service-cognition/openapi.jsonrust/cloud-storage/document_cognition_service/src/api/context/mod.rsrust/cloud-storage/document_cognition_service/src/api/context/test.rsrust/cloud-storage/document_cognition_service/src/api/stream/chat_message/mod.rsrust/cloud-storage/document_cognition_service/src/main.rsrust/cloud-storage/macro_db_client/migrations/20260610212946_add_team_id_to_memory_table.sqlrust/cloud-storage/memory/src/domain/mod.rsrust/cloud-storage/memory/src/domain/ports.rsrust/cloud-storage/memory/src/domain/service.rsrust/cloud-storage/memory/src/domain/service/team.rsrust/cloud-storage/memory/src/domain/service/team/test.rsrust/cloud-storage/memory/src/inbound/axum_router.rsrust/cloud-storage/memory/src/main.rsrust/cloud-storage/memory/src/outbound.rsrust/cloud-storage/memory/src/outbound/pg_memory_repo.rsrust/cloud-storage/memory/src/outbound/pg_team_memory_repo.rsrust/cloud-storage/memory/src/outbound/pg_team_memory_repo/test.rsrust/cloud-storage/scheduled_action/src/outbound/inprocess_executor/agent_task.rs
Comment on lines
+107
to
+119
| async fn get_or_generate_memory( | ||
| &self, | ||
| user: macro_user_id::user_id::MacroUserIdStr<'static>, | ||
| ) -> super::Result<Memories> { | ||
| let (user_memory, team_memory) = tokio::join!( | ||
| self.get_or_generate_user_memory(user.clone()), | ||
| self.get_or_generate_team_memory(user) | ||
| ); | ||
|
|
||
| Ok(Memories { | ||
| user: user_memory?, | ||
| team: team_memory?, | ||
| }) |
Contributor
There was a problem hiding this comment.
Don't make combined memory retrieval all-or-nothing.
tokio::join! is fine here, but the immediate ? on both results means a transient failure in either branch drops the other successful memory on the floor. That turns /memory into a 500 even when one scope was available, which defeats the new "return whichever memories currently exist" contract.
Suggested direction
- let (user_memory, team_memory) = tokio::join!(
+ let (user_memory, team_memory) = tokio::join!(
self.get_or_generate_user_memory(user.clone()),
self.get_or_generate_team_memory(user)
);
- Ok(Memories {
- user: user_memory?,
- team: team_memory?,
- })
+ match (user_memory, team_memory) {
+ (Ok(user), Ok(team)) => Ok(Memories { user, team }),
+ (Ok(user), Err(e)) => {
+ tracing::error!(error = ?e, "failed to load team memory");
+ Ok(Memories { user, team: None })
+ }
+ (Err(e), Ok(team)) => {
+ tracing::error!(error = ?e, "failed to load user memory");
+ Ok(Memories { user: None, team })
+ }
+ (Err(user_err), Err(team_err)) => {
+ tracing::error!(error = ?user_err, "failed to load user memory");
+ tracing::error!(error = ?team_err, "failed to load team memory");
+ Err(user_err)
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/memory/src/domain/service.rs` around lines 107 - 119, In
get_or_generate_memory, avoid treating the joined results from tokio::join! as
all-or-nothing: instead of immediately using the `?` on both results, handle
each Result returned by get_or_generate_user_memory and
get_or_generate_team_memory individually so one Err does not discard the other's
Ok; call both functions with tokio::join!, map each result to either a
successful memory or None / a fallback value, log or propagate errors
selectively if needed, and construct Memories { user: ..., team: ... } using the
available values so the endpoint can return whichever scope(s) exist.
Comment on lines
+99
to
+140
| let env = Environment::new_or_prod(); | ||
| if needs_generation && !matches!(env, Environment::Local) { | ||
| let previous_memory = record.as_ref().map(|r| r.memory.clone()); | ||
| let pool = self.db.clone(); | ||
| let tool_context = self.tool_context.clone(); | ||
| let toolset = self.tools.toolset.clone(); | ||
| let prompt: Box<dyn std::fmt::Display + Send + Sync> = | ||
| Box::new(self.tools.prompt.to_string()); | ||
| tokio::spawn(async move { | ||
| // Unlike personal memory, every member of a (possibly large) | ||
| // team can trigger a regeneration while one is already in | ||
| // flight, so generations are serialized cross-instance via a | ||
| // Postgres advisory lock. | ||
| let _lock = match try_acquire_generation_lock(&pool, team_id).await { | ||
| Ok(Some(lock)) => lock, | ||
| Ok(None) => { | ||
| tracing::debug!(%team_id, "team memory generation already in progress"); | ||
| return; | ||
| } | ||
| Err(e) => { | ||
| tracing::error!(error = ?e, %team_id, "failed to acquire team memory generation lock"); | ||
| return; | ||
| } | ||
| }; | ||
| let repo = crate::outbound::pg_memory_repo::PgMemoryRepo::new(pool.clone()); | ||
| let team_repo = | ||
| crate::outbound::pg_team_memory_repo::PgTeamMemoryRepo::new(pool.clone()); | ||
| let tools = ToolSetWithPrompt { toolset, prompt }; | ||
| let svc = MemoryServiceImpl::new(pool, repo, team_repo, tool_context, tools); | ||
| match svc | ||
| .generate_team_memory(team_id, user.clone(), previous_memory) | ||
| .await | ||
| { | ||
| Ok(_) => tracing::info!(%team_id, "team memory generated"), | ||
| Err(MemoryError::Rejected(reason)) => { | ||
| tracing::warn!(%team_id, %reason, "team memory rejected by judge") | ||
| } | ||
| Err(e) => { | ||
| tracing::error!(%team_id, error = ?e, "team memory generation failed") | ||
| } | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Re-check staleness after taking the advisory lock.
needs_generation and previous_memory are captured before the spawned task acquires the lock. If another request refreshes the row first, a delayed task can still grab the lock afterward and run a second full generation based on stale state. That turns the lock into serialization only, not deduplication, and can overwrite a fresh summary with a redundant pass.
Suggested direction
tokio::spawn(async move {
// Unlike personal memory, every member of a (possibly large)
// team can trigger a regeneration while one is already in
// flight, so generations are serialized cross-instance via a
// Postgres advisory lock.
let _lock = match try_acquire_generation_lock(&pool, team_id).await {
Ok(Some(lock)) => lock,
Ok(None) => {
tracing::debug!(%team_id, "team memory generation already in progress");
return;
}
Err(e) => {
tracing::error!(error = ?e, %team_id, "failed to acquire team memory generation lock");
return;
}
};
let repo = crate::outbound::pg_memory_repo::PgMemoryRepo::new(pool.clone());
let team_repo =
crate::outbound::pg_team_memory_repo::PgTeamMemoryRepo::new(pool.clone());
+ let latest = match team_repo.get_latest_team_memory(team_id).await {
+ Ok(latest) => latest,
+ Err(e) => {
+ tracing::error!(error = ?e, %team_id, "failed to reload latest team memory");
+ return;
+ }
+ };
+ let still_needs_generation = match &latest {
+ Some(r) => {
+ let age = Utc::now() - r.updated_at;
+ age > chrono::Duration::from_std(MAX_AGE).unwrap_or(chrono::TimeDelta::MAX)
+ }
+ None => true,
+ };
+ if !still_needs_generation {
+ tracing::debug!(%team_id, "team memory already refreshed");
+ return;
+ }
+ let previous_memory = latest.as_ref().map(|r| r.memory.clone());
let tools = ToolSetWithPrompt { toolset, prompt };
let svc = MemoryServiceImpl::new(pool, repo, team_repo, tool_context, tools);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/memory/src/domain/service/team.rs` around lines 99 - 140,
The spawned task captures needs_generation and previous_memory before acquiring
the advisory lock, allowing a stale generation to proceed; after acquiring the
lock via try_acquire_generation_lock(&pool, team_id).await but before calling
svc.generate_team_memory(...), re-fetch the current row/state (the same source
used to compute needs_generation/previous_memory) and re-check whether
generation is still required and whether previous_memory changed, and only call
MemoryServiceImpl::generate_team_memory(team_id, user.clone(), previous_memory)
when the re-check indicates generation is still needed; if not needed, log and
return. Ensure this re-check uses the same repo/db pool and the same semantics
as the original needs_generation computation so the advisory lock enforces
deduplication rather than just serialization.
Comment on lines
+128
to
+130
| match svc | ||
| .generate_team_memory(team_id, user.clone(), previous_memory) | ||
| .await |
Contributor
There was a problem hiding this comment.
Shared team memory is generated with one member's permissions.
This flow researches workspace artifacts using the triggering user, then persists the result under team_id and serves that same summary to every teammate. If one member can see private docs/channels/emails that another teammate cannot, the generated team memory becomes a cross-user data leak.
Also applies to: 146-153, 209-210
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/cloud-storage/memory/src/domain/service/team.rs` around lines 128 - 130,
The code calls svc.generate_team_memory(team_id, user.clone(), previous_memory)
using the triggering user's permissions, which risks leaking artifacts private
to that user into a shared team memory; update the call and implementation to
use a team-scoped visibility check instead of a single user's view by (a)
changing generate_team_memory to accept the team's membership list or a
team-permission context (e.g., team_id plus Vec<member_ids> or a TeamPermission
object) and (b) inside generate_team_memory only include artifacts visible to
every teammate (compute intersection of visible artifact IDs) or otherwise
restrict to artifacts explicitly marked as team-visible; update every call site
(the current svc.generate_team_memory invocations and analogous calls at the
other noted locations) to pass the team membership/permission context so the
persisted team memory contains only data safe for all members.
Comment on lines
125
to
131
| let mut system_prompt = match memories.user { | ||
| Some(memory) => format!( | ||
| "{}\n{}\n<user_memory>\n{}\n</user_memory>\n{}", | ||
| tools.prompt, SCHEDULED_AGENT_PROMPT, memory, agent_task.prompt | ||
| "{}\n{}\n<user_memory>\n{}\n</user_memory>", | ||
| tools.prompt, SCHEDULED_AGENT_PROMPT, memory | ||
| ), | ||
| None => format!("{}\n{}", tools.prompt, agent_task.prompt), | ||
| None => tools.prompt.to_string(), | ||
| }; |
Contributor
There was a problem hiding this comment.
Preserve SCHEDULED_AGENT_PROMPT when user memory is missing.
Line 130 drops SCHEDULED_AGENT_PROMPT in the None branch, so scheduled runs without personal memory lose required guardrail instructions.
Suggested fix
- let mut system_prompt = match memories.user {
- Some(memory) => format!(
- "{}\n{}\n<user_memory>\n{}\n</user_memory>",
- tools.prompt, SCHEDULED_AGENT_PROMPT, memory
- ),
- None => tools.prompt.to_string(),
- };
+ let mut system_prompt = format!("{}\n{}", tools.prompt, SCHEDULED_AGENT_PROMPT);
+ if let Some(memory) = memories.user {
+ system_prompt.push_str(&format!("\n<user_memory>\n{memory}\n</user_memory>"));
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@rust/cloud-storage/scheduled_action/src/outbound/inprocess_executor/agent_task.rs`
around lines 125 - 131, The None branch for building system_prompt drops
SCHEDULED_AGENT_PROMPT so scheduled runs lose guardrails; update the None branch
that currently returns tools.prompt.to_string() to include
SCHEDULED_AGENT_PROMPT (e.g., concatenate tools.prompt and
SCHEDULED_AGENT_PROMPT with a newline) so system_prompt always contains both
tools.prompt and SCHEDULED_AGENT_PROMPT even when memories.user is None; modify
the code around system_prompt, memories.user, SCHEDULED_AGENT_PROMPT and
tools.prompt accordingly.
- get_or_generate_memory returns whichever scope resolved instead of failing both when one errors - re-check team memory staleness after acquiring the advisory lock so it deduplicates rather than merely serializes generations - scheduled agent runs keep SCHEDULED_AGENT_PROMPT when no user memory exists - document the team memory repo trait and Postgres impl https://claude.ai/code/session_01CyfX47q3gqFp49pLrcco8b
Contributor
Author
|
Status of the review findings as of 73e1af5:
Generated by Claude Code |
jbecke
changed the title
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements team-level memory generation and retrieval, allowing AI agents to access shared context about a team's work, members, and processes. This complements the existing per-user memory system by providing team-wide background knowledge to all team members.
Key Changes
Team Memory Service (
team_service.rs): New service that generates and retrieves team memories using an AI agent loop. Includes:get_or_generate_team_memory(): Fetches latest team memory or triggers background generation if stale (>24h) or missinggenerate_team_memory(): Uses AI agent with tool access to research team workspace (documents, projects, emails, channels) and generate ~1000-3000 word team summaryTeam Memory Repository (
pg_team_memory_repo.rs): PostgreSQL-backed repository implementingTeamMemoryRepotrait with:save_team_memory(): Upserts team memory (one per team)get_latest_team_memory(): Retrieves current team memory with timestampget_team_memory_by_id(): Fetches specific memory versionget_user_team_id(): Resolves user's team membershipget_team_overview(): Fetches team name and member list for grounding generationHTTP API (
axum_router.rs): New/memory/teamendpoint that:Database Schema (
20260610212946_create_team_memory_table.sql): Newteam_memorytable with:team_id)Integration Points:
Comprehensive Tests: Unit tests for repository operations, system prompt building, and edge cases (team isolation, missing teams, stale data)
Notable Implementation Details
https://claude.ai/code/session_01CyfX47q3gqFp49pLrcco8b