Seal background agent grants and push capabilities#208
Conversation
Introduce the encrypted embedding storage and retrieval primitives on top of the existing schema so later agent features can build on a stable RAG base. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Introduce the main Maple agent runtime, persistence models, and hidden backing conversation flow so the restacked branch has a working end-to-end agent foundation on top of the new schema. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Expand Maple with paginated history and reset controls, Brave-backed web search, and encrypted mobile push delivery so the runtime can surface richer results and notify clients off-thread. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> fix(push): avoid FCM and Tinfoil router 0 bind conflict Move FCM onto its own loopback IP so the push and Tinfoil router forwarders can both bind cleanly on agent-rewrite. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Switch Maple creation from implicit lazy bootstrapping to an explicit init flow that seeds onboarding messages and stores user locale and timezone hints for later agent runs. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Add background Maple wakeups with durable leases and retry handling, and let the agent update validated user preferences that can influence schedule timing and future replies. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Let Maple store lightweight reply reactions and require persisted message identifiers in its SSE flow so clients can reconcile streamed output with durable conversation state. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
WalkthroughAdds background-grant and schedule persistence, push device and delivery flows, RAG embeddings and search, and agent runtime, scheduling, and web API updates. Also adjusts response storage, module exports, startup wiring, deployment docs, and build metadata. ChangesPush, RAG, and agent stack
Sequence Diagram(s)sequenceDiagram
participant enqueue_notification
participant NotificationDelivery
participant start_push_worker
participant send_apns_notification
participant send_fcm_notification
enqueue_notification->>NotificationDelivery: insert notification rows
start_push_worker->>NotificationDelivery: lease_pending batch
start_push_worker->>send_apns_notification: send iOS delivery
start_push_worker->>send_fcm_notification: send Android delivery
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| output.push_str(&format!( | ||
| " {}{}\n", | ||
| short_desc, | ||
| if desc.len() > 200 { "..." } else { "" } |
There was a problem hiding this comment.
🟡 Weather alert text falsely appends ellipsis for non-ASCII descriptions
The truncation indicator checks byte length (desc.len() > 200 at src/brave.rs:575) but the actual truncation uses character count (desc.chars().take(200) at src/brave.rs:571), so non-ASCII descriptions that fit in 200 characters but exceed 200 bytes get "..." appended even though they were not actually truncated.
Impact: Weather alert descriptions containing multi-byte characters (e.g., accented letters, CJK text) display a misleading "..." suffix suggesting truncated content that is actually shown in full.
Byte-count vs character-count mismatch in truncation guard
desc.chars().take(200) limits to 200 Unicode scalar values, but desc.len() counts UTF-8 bytes. For a string like 150 emoji characters (each 4 bytes), desc.len() returns 600, so the > 200 guard fires and appends "..." despite the full text being included. The fix is to use desc.chars().count() > 200 for the guard condition to match the truncation logic.
| if desc.len() > 200 { "..." } else { "" } | |
| if desc.chars().count() > 200 { "..." } else { "" } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| let apns_client = reqwest::Client::builder() | ||
| .timeout(std::time::Duration::from_secs(20)) | ||
| .http2_prior_knowledge() | ||
| .build()?; |
There was a problem hiding this comment.
🚩 APNs client uses http2_prior_knowledge() which skips HTTP/1.1 upgrade
The APNs push transport at src/push/mod.rs:277 uses http2_prior_knowledge() to force HTTP/2 without trying HTTP/1.1 first. APNs requires HTTP/2, so this is intentional. With the native-tls-alpn feature enabled in Cargo.toml:48, TLS ALPN negotiation should still occur during the handshake, and the HTTP/2 prior knowledge flag only affects the HTTP-layer protocol selection. The regular FCM client correctly does NOT use this flag since FCM supports both protocols. Worth confirming this works correctly through the vsock proxy tunnel to api.push.apple.com, which terminates TLS at the remote endpoint.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if let Some(agent_conversation_id) = agent_conversation_id { | ||
| use crate::models::schema::conversations::dsl::*; | ||
| use diesel::prelude::*; | ||
|
|
||
| diesel::delete( | ||
| conversations | ||
| .filter(user_id.eq(user.uuid)) | ||
| .filter(id.ne(agent_conversation_id)), | ||
| ) | ||
| .execute(&mut conn) | ||
| .map_err(|_| ApiError::InternalServerError)?; |
There was a problem hiding this comment.
🚩 Delete-all-conversations now preserves main agent but deletes subagent conversations
At src/web/responses/conversations.rs:816-826, the delete-all-conversations handler excludes the main agent's conversation from deletion but does NOT exclude subagent conversations. This means calling 'delete all conversations' deletes all subagent conversations, which cascade-deletes their agent rows (via ON DELETE CASCADE on agents.conversation_id), their schedules, schedule runs, and background grants. The main agent survives but loses all subagents. This is a significant behavioral change from the previous delete-all behavior and may be surprising to users who have active scheduled tasks on subagents.
Was this helpful? React with 👍 or 👎 to provide feedback.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| let salt = generate_random::<32>(); | ||
| let hkdf = Hkdf::<Sha256>::new(Some(&salt), shared_secret.raw_secret_bytes().as_slice()); | ||
| let mut key = [0_u8; 32]; |
| ephemeral_public.as_affine(), | ||
| ); | ||
| let hkdf = Hkdf::<Sha256>::new(Some(&salt), shared_secret.raw_secret_bytes().as_slice()); | ||
| let mut key = [0_u8; 32]; |
| let body = build_fcm_payload(event, push_token, preview_payload); | ||
| let response = transport | ||
| .client | ||
| .post(format!( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (26)
src/rag.rs-515-549 (1)
515-549: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdvance
last_idbefore anycontinue.Right now the cursor only moves after
out.push(...). If a batch contains only dim-mismatched rows, every iteration hitscontinue,last_idstays unchanged, and the outer loop re-reads the same batch forever.Suggested fix
for row in rows { let EmbeddingScanRow { source_type, conversation_id, vector_enc, content_enc, token_count, vector_dim, id, } = row; + last_id = id; let vector_bytes = decrypt_with_key(user_key, &vector_enc) .map_err(|_| ApiError::InternalServerError)?; let vector = deserialize_f32_le(&vector_bytes)?; if vector.len() != vector_dim as usize { debug!( "Skipping embedding id={} for user={} due to dim mismatch (expected={}, got={})", id, user_id, vector_dim, vector.len() ); continue; } out.push(CachedEmbedding { source_type, conversation_id, vector, content_enc, token_count, }); - last_id = id; }Also applies to: 625-659
🤖 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 `@src/rag.rs` around lines 515 - 549, The cursor advancement in the row-processing loop of the embedding scan is happening only after successful pushes, so dim-mismatched rows can cause the batch cursor to never move and the outer pagination loop to repeat the same rows indefinitely. Update the loop in the embedding loading logic (the one destructuring EmbeddingScanRow and updating last_id) so last_id is advanced for every processed row before any early continue paths, including the vector-dimension mismatch branch, and apply the same fix in the other duplicated range mentioned in the comment.src/rag.rs-488-503 (1)
488-503: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFilter searches to the active embedding model.
search_user_embeddings()always embeds the query withDEFAULT_EMBEDDING_MODEL, but both loaders pull every row for the user. Older embeddings with the same dimension will still be scored, so stale vectors can outrank current ones even thoughembeddings_status()already treats them as stale.Suggested fix
let rows: Vec<EmbeddingScanRow> = user_embeddings::table .filter(user_embeddings::user_id.eq(user_id)) + .filter(user_embeddings::embedding_model.eq(DEFAULT_EMBEDDING_MODEL)) .filter(user_embeddings::id.gt(last_id))let mut query = user_embeddings::table .filter(user_embeddings::user_id.eq(user_id)) + .filter(user_embeddings::embedding_model.eq(DEFAULT_EMBEDDING_MODEL)) .filter(user_embeddings::id.gt(last_id)) .filter(user_embeddings::tags_enc.overlaps_with(tags_enc_filter.clone())) .into_boxed();Also applies to: 600-613
🤖 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 `@src/rag.rs` around lines 488 - 503, `search_user_embeddings()` is loading rows without restricting them to the active embedding model, so stale vectors can still be scored against the `DEFAULT_EMBEDDING_MODEL` query. Update both embedding loaders in `rag.rs` (the `user_embeddings::table` query blocks near the `load` calls) to filter by the active model identifier before selecting/scoring rows, and keep the stale-embedding behavior aligned with `embeddings_status()`.src/web/agent/schedules.rs-1295-1328 (1)
1295-1328: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRecord idempotency before retryable scheduled side effects can duplicate.
Scheduled tool calls and assistant messages are persisted before the run has a durable output marker tied to the lease. If the worker crashes or loses the lease after a tool side effect/message insert but before
record_output, the retry path can execute the same turn again.🤖 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 `@src/web/agent/schedules.rs` around lines 1295 - 1328, The scheduled run flow in the persisted tool call and assistant message loop can duplicate side effects because `insert_tool_call_and_output`, `insert_assistant_message`, and `AgentScheduleRun::record_output` are not guarded by a durable idempotency marker first. Update `src/web/agent/schedules.rs` so `record_output` (or an equivalent lease-bound idempotency check) happens before any retryable side effects in the same turn, and make the `result.executed_tools` / `result.messages` processing skip or short-circuit when the output marker already exists.src/web/agent/schedules.rs-1011-1042 (1)
1011-1042: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTransition runs on background-grant preflight failures.
Grant decrypt, background key derivation, instruction decrypt, and policy verification errors return from
process_leased_schedule_runwithoutmark_retry/mark_failed. The run will just become lease-expired and repeat without attempt accounting.🤖 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 `@src/web/agent/schedules.rs` around lines 1011 - 1042, The background-grant preflight path in process_leased_schedule_run is returning early on decrypt, key-derivation, instruction-decrypt, and policy-check failures without updating run state. Update the error handling around decrypt_background_grant_v1, get_user_key_for_agent_background_grant, decrypt_string, and verify_scheduled_policy so these failures call the same retry/failure accounting used elsewhere, rather than just bubbling out and letting the lease expire.src/web/agent/mod.rs-455-458 (1)
455-458: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftAvoid full conversation scans for single-item lookup and reaction updates.
get_item_from_conversationfetchesi64::MAXmessages, then scans in memory; reaction updates call it twice. Large conversations can make single item GET/reaction paths expensive. Query byconversation_id + user_id + item_uuiddirectly instead.Also applies to: 477-498
🤖 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 `@src/web/agent/mod.rs` around lines 455 - 458, The single-item lookup path in get_item_from_conversation is doing a full conversation fetch via get_conversation_context_messages with i64::MAX and then scanning in memory, and reaction update flows are calling it multiple times. Update get_item_from_conversation and the reaction update callers to query the item directly by conversation_id, user_id, and item_uuid instead of loading all messages, using the existing database access layer and error_mapping::map_message_error for failures.src/web/agent/mod.rs-738-765 (1)
738-765: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake main-agent deletion atomic.
This deletes subagent conversations, the main conversation, and memory blocks in separate writes. A mid-way error leaves partially deleted agent state. Wrap the DB mutations in one transaction, then evict
rag_cacheonly after commit.🤖 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 `@src/web/agent/mod.rs` around lines 738 - 765, Make the main-agent deletion flow atomic by moving the conversation and memory-block mutations in `delete_main_agent` into a single database transaction instead of separate writes. Use the existing DB connection from `state.db.get_pool().get()` to begin a transaction, then perform `Agent::get_main_for_user`, `Agent::list_subagents_for_user`, `delete_conversation_for_user`, and `MemoryBlock::delete_all_for_user` against that transaction. After the transaction commits successfully, evict `rag_cache`; keep the current error logging paths but ensure any failure aborts the transaction so partial deletions cannot persist.src/web/agent/tools.rs-590-631 (1)
590-631: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winClamp tool-controlled search limits before querying embeddings.
limitandtop_kcome directly from tool args, so the model can request very large searches and inflate DB/vector work plus prompt output. Clamp to a small maximum before callingsearch_user_embeddings.Proposed fix
- let limit: usize = args.get("limit").and_then(|l| l.parse().ok()).unwrap_or(5); + let limit: usize = args + .get("limit") + .and_then(|l| l.parse::<usize>().ok()) + .unwrap_or(5) + .clamp(1, 20); ... - let top_k: usize = args.get("top_k").and_then(|k| k.parse().ok()).unwrap_or(5); + let top_k: usize = args + .get("top_k") + .and_then(|k| k.parse::<usize>().ok()) + .unwrap_or(5) + .clamp(1, 20);Also applies to: 790-814
🤖 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 `@src/web/agent/tools.rs` around lines 590 - 631, The tool search path currently passes user-controlled `limit`/`top_k` values straight into `search_user_embeddings`, which can make embedding queries too large. In the tool handlers around the `limit` parsing and `search_user_embeddings` calls, clamp both values to a small fixed maximum before use, and apply the same change to the other affected search block noted in the review. Keep the existing defaults, but ensure the final value passed to `search_user_embeddings` cannot exceed the cap.src/web/agent/runtime.rs-485-510 (1)
485-510: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOnly repair on
ConversationNotFound.The
Err(_)arm treats every lookup failure as “conversation missing”. A transient DB error here will create a new conversation and repoint the main agent away from its real thread. MatchResponsesError::ConversationNotFoundexplicitly and surface all other errors.🤖 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 `@src/web/agent/runtime.rs` around lines 485 - 510, In runtime.rs, the fallback in the Conversation::get_by_id_and_user lookup is too broad because Err(_) treats every failure as a missing conversation. Update the match in the main agent repair flow to handle only ResponsesError::ConversationNotFound, and let all other errors propagate or return an internal error without creating a new conversation. Keep the repair logic in the same branch that recreates the Conversation and updates the Agent conversation_id, but only after confirming the specific not-found case.src/web/agent/runtime.rs-484-548 (1)
484-548: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake agent bootstrap/create atomic.
init_main_agentandcreate_subagenteach perform multi-row writes without a transaction. Any failure after the conversation insert leaves partial state behind; for the main agent, that can also leak an orphan conversation through the public Conversations API becausesrc/web/responses/conversations.rshides it only whenAgent::get_main_for_usersucceeds. Wrap the conversation insert, agent insert/update, preference writes, and onboarding seed in one transaction.Also applies to: 578-610
🤖 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 `@src/web/agent/runtime.rs` around lines 484 - 548, Make the main agent bootstrap flow atomic by wrapping the conversation insert/update, agent insert/update, user preference upserts, and onboarding seeding in a single transaction. In `init_main_agent`, ensure the `Conversation::get_by_id_and_user` recovery path, `NewConversation::insert`, `diesel::update(...).get_result::<Agent>`, `upsert_user_preference`, and `seed_main_agent_onboarding_messages` all run within one transactional scope so partial writes cannot persist. Apply the same transactional treatment to `create_subagent` as well, using the existing function names to locate the multi-row write paths.src/web/agent/runtime.rs-1584-1585 (1)
1584-1585: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClamp token counts before writing
i32columns.
insert_user_message()already guards this, but the new assistant/tool/summary paths still narrow token counts with rawas i32casts. Large tool outputs or model replies can wrap into bogus negative values and poison compaction/accounting.Also applies to: 1671-1681, 1835-1835
🤖 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 `@src/web/agent/runtime.rs` around lines 1584 - 1585, Clamp the token count before storing it in the i32-backed message columns, since the assistant/tool/summary paths in the runtime still use raw as i32 casts. Update the token-count handling in the code around the assistant/tool/summary insert paths (including the logic in insert_user_message and the related message construction helpers) so count_tokens values are bounded to i32::MAX or otherwise safely converted before persistence. Ensure the same safe conversion is used consistently wherever completion_tokens or similar counts are written.src/web/agent/runtime.rs-1731-1877 (1)
1731-1877: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease the DB connection before the embedding call.
maybe_compact()keepsconnchecked out while awaitingget_embedding_vector(...). A slow embedding request now holds a pool slot for its entire network round-trip, which can starve unrelated chats under load. Read first, drop/reacquire around the await, then insert the summary row.🤖 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 `@src/web/agent/runtime.rs` around lines 1731 - 1877, Release the checked-out DB connection before awaiting the embedding request in maybe_compact() so the pool slot is not held during the network round-trip. Use the existing conn usage around ConversationSummary::get_latest_for_conversation, RawThreadMessageMetadata::get_conversation_context_metadata, and RawThreadMessage::get_messages_by_ids to read everything needed first, then drop or scope conn before calling crate::web::get_embedding_vector, and reacquire a fresh connection afterward for new_summary.insert.src/web/agent/signatures.rs-409-417 (1)
409-417: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not log plaintext prompts.
Lines 414-415 emit full system/user prompts, which include decrypted conversation history, memory blocks, and tool results. Keep only metadata such as lengths/kind.
Proposed fix
fn trace_generated_prompt(prompt_kind: &str, system_prompt: &str, user_prompt: &str) { trace!( prompt_kind = %prompt_kind, system_prompt_len = system_prompt.chars().count(), user_prompt_len = user_prompt.chars().count(), - system_prompt = system_prompt, - user_prompt = user_prompt, "Generated LM prompt" ); }🤖 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 `@src/web/agent/signatures.rs` around lines 409 - 417, The trace_generated_prompt function is logging full plaintext system_prompt and user_prompt values, which can expose sensitive conversation data; remove those fields from the trace! call and keep only metadata such as prompt_kind and the prompt length counts. Update the logging in trace_generated_prompt so it records the kind and lengths only, without emitting decrypted history, memory blocks, or tool results.src/brave.rs-504-508 (1)
504-508: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winBound unknown rich-result payloads before adding them to agent context.
Line 507 pretty-prints the entire rich callback blob for unknown subtypes. A large provider payload can bloat
web_searchtool output and exhaust downstream LLM context/cost; cap this fallback before formatting it.Proposed fix
+const MAX_RICH_FALLBACK_CHARS: usize = 4_000; + +fn truncate_for_prompt(value: &str, max_chars: usize) -> String { + let mut truncated: String = value.chars().take(max_chars).collect(); + if value.chars().count() > max_chars { + truncated.push_str("\n...[truncated]"); + } + truncated +} + impl RichResult { pub fn format(&self) -> Option<String> { match self.subtype.as_deref()? { @@ "unit_conversion" => self.format_unit_conversion(), "definitions" => self.format_definition(), - subtype => Some(format!( - "{}\n{}", - subtype, - serde_json::to_string_pretty(&self.data).unwrap_or_default() - )), + subtype => { + let pretty = serde_json::to_string_pretty(&self.data).unwrap_or_default(); + Some(format!( + "{}\n{}", + subtype, + truncate_for_prompt(&pretty, MAX_RICH_FALLBACK_CHARS) + )) + } } }🤖 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 `@src/brave.rs` around lines 504 - 508, The fallback in the unknown rich-result branch currently serializes the full payload in the rich callback path, which can bloat agent context. Update the logic in the matching arm of the rich-result formatter to bound the data before calling serde_json::to_string_pretty on self.data, using an existing truncation or size-limiting helper if available, and keep the subtype-based formatting behavior intact.migrations/2026-06-26-224528_agent_background_push_aead/up.sql-55-59 (1)
55-59: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the
background_grant_id/source_kindinvariant in the schema.
src/push/mod.rsonly emits a grant id foragent_background, and the worker later resolves that id back to anAgentBackgroundGrant. This migration acceptssource_kind = 'agent_background'withbackground_grant_id = NULL, so a bad insert can create an event the delivery path cannot attribute correctly. Add a CHECK that requiresbackground_grant_id IS NOT NULLforagent_backgroundandIS NULLforrequest_continuation.Suggested constraint
ALTER TABLE notification_events ADD COLUMN source_kind TEXT NOT NULL DEFAULT 'request_continuation' CHECK (source_kind IN ('request_continuation', 'agent_background')), ADD COLUMN source_request_id UUID, - ADD COLUMN background_grant_id BIGINT REFERENCES agent_background_grants(id) ON DELETE SET NULL; + ADD COLUMN background_grant_id BIGINT REFERENCES agent_background_grants(id) ON DELETE SET NULL, + ADD CONSTRAINT notification_events_background_source_check CHECK ( + (source_kind = 'agent_background' AND background_grant_id IS NOT NULL) + OR + (source_kind = 'request_continuation' AND background_grant_id IS NULL) + );🤖 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 `@migrations/2026-06-26-224528_agent_background_push_aead/up.sql` around lines 55 - 59, The notification_events schema currently allows source_kind and background_grant_id combinations that break the delivery attribution path from src/push/mod.rs and the worker’s AgentBackgroundGrant lookup. Update the migration to add a CHECK constraint on notification_events that enforces background_grant_id IS NOT NULL when source_kind = 'agent_background' and background_grant_id IS NULL when source_kind = 'request_continuation', using the existing source_kind/background_grant_id columns in the same ALTER TABLE block.migrations/2026-06-26-224528_agent_background_push_aead/down.sql-32-37 (1)
32-37: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRollback will fail once any
agent_backgroundseed wrap exists.This restores the old
user_seed_wrappings_credential_kind_check, but the PR also introducescredential_kind = 'agent_background'. Those rows are never deleted here, so theADD CONSTRAINTwill start failing as soon as the feature is exercised.Minimal rollback fix
+DELETE FROM user_seed_wrappings +WHERE credential_kind = 'agent_background'; + ALTER TABLE user_seed_wrappings DROP CONSTRAINT IF EXISTS user_seed_wrappings_credential_kind_check;🤖 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 `@migrations/2026-06-26-224528_agent_background_push_aead/down.sql` around lines 32 - 37, The rollback in the down migration for user_seed_wrappings_credential_kind_check re-adds an enum check that excludes agent_background, so it will fail if any such rows exist. Update the down.sql migration to remove or convert existing agent_background rows before re-adding the constraint, or otherwise make the rollback safe for the new credential_kind value introduced by the migration. Use the user_seed_wrappings table and the credential_kind check constraint as the key points to adjust.src/agent_background.rs-68-75 (1)
68-75: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire
may_send_pushfor full scheduled-agent grants.
verify_scheduled_policyaccepts a grant withmay_send_push == falseas long as the capability class isfull_scheduled_agent, so a grant that denies push can still pass the scheduled-agent policy check.Proposed fix
&& self.schedule_uuid == schedule_uuid && self.instruction_hash == instruction_hash && self.may_decrypt_user_content + && self.may_send_push && self.capability_class == AGENT_BACKGROUND_CAPABILITY_FULL_SCHEDULED_AGENT🤖 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 `@src/agent_background.rs` around lines 68 - 75, The scheduled-agent policy check in verify_scheduled_policy currently allows full_scheduled_agent grants even when may_send_push is false. Update the grant validation in AgentBackgroundGrant so full scheduled-agent grants must also require may_send_push, alongside the existing version, UUID, instruction_hash, may_decrypt_user_content, and capability_class checks.src/push/apns.rs-112-139 (1)
112-139: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMap
400/BadDeviceTokentoInvalidToken.
BadDeviceTokenis non-retryable and should retire the stored device token, but it currently falls through toFailed, so the worker may keep targeting an invalid token.🤖 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 `@src/push/apns.rs` around lines 112 - 139, Update the APNS status handling in the push outcome mapping so `400` with `BadDeviceToken` is treated as `PushSendOutcome::InvalidToken` rather than falling through to `PushSendOutcome::Failed`. In the `match status.as_u16()` logic inside the APNS response handling, add a branch for this provider error alongside the existing `410` invalid-token case, preserving `provider_status_code` and using the existing `error_message` value so the stored device token is retired correctly.src/push/crypto.rs-19-20 (1)
19-20: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize this field as
v
#[serde(alias = "v")]only adds an alternate deserialization name; the serialized JSON key is stillenc_v. Userenameif the wire contract expectsv.Proposed fix
- #[serde(alias = "v")] + #[serde(rename = "v")] pub enc_v: i32,🤖 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 `@src/push/crypto.rs` around lines 19 - 20, The `enc_v` field in the crypto payload is only using a serde alias, so it still serializes as `enc_v` instead of the expected wire key `v`. Update the `enc_v` definition to use the appropriate serde rename behavior so both serialization and deserialization match the contract, and keep the change scoped to the struct field in `src/push/crypto.rs`.src/push/binding.rs-83-98 (1)
83-98: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRecompute capability hashes during row matching.
matches_device_rowtrusts the plaintextpush_token_hashandnotification_public_key_hashfields without checking they match the raw encryptedpush_tokenandnotification_public_key. Downstream sends use the raw token/key, so an internally inconsistent capability can pass this check.Proposed fix
&& self.environment == device.environment && self.app_id == device.app_id && self.key_algorithm == device.key_algorithm + && hash_bytes(self.push_token.as_bytes()) == self.push_token_hash + && hash_bytes(&self.notification_public_key) == self.notification_public_key_hash && self.push_token_hash == device.push_token_hash && self.notification_public_key_hash == device.notification_public_key_hash && self.supports_encrypted_preview == device.supports_encrypted_preview🤖 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 `@src/push/binding.rs` around lines 83 - 98, matches_device_row currently compares stored hash fields directly, so an inconsistent capability can still match even if the raw push_token or notification_public_key changed. Update PushCapability::matches_device_row to recompute the token/key hashes from the PushDevice values and compare those derived hashes instead of trusting push_token_hash and notification_public_key_hash. Keep the existing checks for the other fields, and use the same hashing logic already used when building or validating a capability so row matching and downstream sends stay consistent.src/models/push_devices.rs-202-214 (1)
202-214: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftGuard invalidation against stale device rows.
invalidaterevokes byidonly. If a device row is updated with a new token/capability while an old delivery is in flight, an invalid-token response for the old token can revoke the freshly updated device. Add an expectedpush_token_hash/capability filter, or use append-only device rows for re-registration.🤖 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 `@src/models/push_devices.rs` around lines 202 - 214, The invalidate logic in PushDevice::invalidate currently revokes rows by id alone, which can accidentally revoke a freshly re-registered device. Update the update query in invalidate to also match the expected push_token_hash or capability (using the same identifiers available to the caller) before setting revoked_at and updated_at, or switch re-registration to append-only device rows so old in-flight invalidations only affect the intended row.src/push/worker.rs-223-238 (1)
223-238: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRenew the delivery lease while waiting on APNs/FCM.
After the DB connection is dropped,
build_send_outcomecan run longer than the 60s lease TTL. Another worker can then re-lease the sameleasedrow and send a duplicate notification before this worker records the terminal state. Mirror the schedule-run heartbeat or bound provider timeouts below the lease TTL with margin.🤖 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 `@src/push/worker.rs` around lines 223 - 238, Renew the delivery lease while the send is in flight: in the push delivery flow around build_send_outcome and the subsequent DB checkout on state.db.get_pool().get(), keep the leased row heartbeated or otherwise extend its lease before waiting on APNs/FCM so it cannot expire mid-send. Use the existing scheduler heartbeat pattern as the guide, and ensure any provider timeout used by build_send_outcome stays safely below the lease TTL with margin so another worker cannot re-lease the same delivery before the terminal state is recorded.src/push/worker.rs-523-530 (1)
523-530: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep background-grant lookup failures retryable.
Diesel errors from
AgentBackgroundGrant::get_by_idare mapped toCryptoError;classify_internal_push_errortreats that as permanent failure, so a transient DB query error drops the notification instead of retrying. Map DB lookup errors to a retryable DB error variant and reserveCryptoErrorfor missing/revoked/mismatched grant data.🤖 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 `@src/push/worker.rs` around lines 523 - 530, The background-grant lookup in AgentBackgroundGrant::get_by_id is mapping Diesel query failures to PushError::CryptoError, which makes classify_internal_push_error treat transient DB issues as permanent. Update the error mapping in this lookup path so database/query failures become a retryable DB-related PushError variant, and keep PushError::CryptoError only for true grant-data problems like missing, revoked, or mismatched grants. Make sure the retry classification still routes transient failures through the retry path without changing the successful grant retrieval behavior.src/models/agent_schedule_runs.rs-145-169 (1)
145-169: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake output persistence and the schedule-run marker atomic.
src/web/agent/schedules.rspersists the assistant message before callingrecord_output; if the lease expires between those writes, the message remains stored whileoutput_countstays0, so the next lease can re-run the turn instead of taking the partial-output recovery path. Persist the assistant message and run marker in one transaction/connection, or add an idempotency marker written with the message.🤖 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 `@src/models/agent_schedule_runs.rs` around lines 145 - 169, The assistant message write and the schedule-run marker update are currently separate operations, which can leave persisted output without a corresponding run marker if the lease changes mid-flight. Update the flow around record_output in agent_schedule_runs::record_output and the caller in agent/web/schedules.rs so both writes happen atomically in one transaction or are protected by a shared idempotency marker written with the message. Make sure the persisted message and output_count/first_output_at changes can’t diverge on retry.src/web/platform/project_routes.rs-904-908 (1)
904-908: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMerge omitted push settings instead of overwriting them with defaults.
Line 904 persists
encrypted_preview_enableddirectly from a request type that defaults omitted values tofalse, and Lines 906-907 persist omitted platform blocks asNone. A partial update can silently disable encrypted previews or clear iOS/Android settings. Load the existing settings and merge supplied fields, or require full replacement payloads.🤖 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 `@src/web/platform/project_routes.rs` around lines 904 - 908, The PushSettings construction in project_routes should not overwrite existing values with defaults when the update request omits fields. Update the handler around the push_settings assignment to merge with the currently stored settings instead of directly using update_request.encrypted_preview_enabled, update_request.ios, and update_request.android. Preserve existing encrypted preview and platform configs unless the request explicitly supplies replacements, or alternatively make the API require a full replacement payload.src/push/fcm.rs-174-199 (1)
174-199: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRestrict the FCM OAuth token endpoint.
Line 174 trusts
token_urifrom project-stored JSON and Line 193 posts the signed assertion there. A malicious or compromised project secret can turn the push worker into SSRF and receive the bearer assertion. Use the default Google token URI, or enforce an HTTPS allowlist before sending.Suggested fix
- let token_uri = service_account - .token_uri - .as_deref() - .unwrap_or(DEFAULT_GOOGLE_TOKEN_URI); + let token_uri = service_account + .token_uri + .as_deref() + .unwrap_or(DEFAULT_GOOGLE_TOKEN_URI); + if token_uri != DEFAULT_GOOGLE_TOKEN_URI { + return Err(PushError::InvalidSecret( + "unsupported FCM token_uri".to_string(), + )); + }🤖 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 `@src/push/fcm.rs` around lines 174 - 199, The FCM token exchange in the push flow is using a project-provided token_uri from the service account JSON, which can be abused for SSRF and leaking the signed assertion. Update the token creation logic in the Fcm OAuth flow to ignore arbitrary token_uri values or strictly validate them against an HTTPS allowlist before calling transport.client.post. Use the existing FcmJwtClaims and the token_uri handling near the JWT assertion request to keep the endpoint fixed to the Google token URL unless it passes validation.src/web/login_routes.rs-258-275 (1)
258-275: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not report logout success when requested push revocation fails.
When
push_device_idis provided, DB pool/update errors are only logged, so the device can remain active after logout. ReturnInternalServerErrorfor DB failures; treatingOk(None)as idempotent is fine.Suggested fix
- match data.db.get_pool().get() { - Ok(mut conn) => { - if let Err(revoke_error) = - PushDevice::revoke_by_uuid_and_user(&mut conn, push_device_id, user_id) - { - error!( - "Failed to revoke push device during logout cleanup: {:?}", - revoke_error - ); - } - } - Err(pool_error) => { - error!( - "Failed to get DB connection during logout push cleanup: {:?}", - pool_error - ); - } - } + let mut conn = data.db.get_pool().get().map_err(|pool_error| { + error!( + "Failed to get DB connection during logout push cleanup: {:?}", + pool_error + ); + ApiError::InternalServerError + })?; + PushDevice::revoke_by_uuid_and_user(&mut conn, push_device_id, user_id).map_err( + |revoke_error| { + error!( + "Failed to revoke push device during logout cleanup: {:?}", + revoke_error + ); + ApiError::InternalServerError + }, + )?;🤖 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 `@src/web/login_routes.rs` around lines 258 - 275, The logout cleanup in the push revocation flow currently logs DB failures but still allows success, so update the logic in the logout handler around PushDevice::revoke_by_uuid_and_user and data.db.get_pool().get() to return InternalServerError when the pool lookup or revoke update fails. Keep Ok(None) as a non-error/idempotent outcome, but ensure any actual DB failure stops the logout success path instead of only emitting an error log.
🟡 Minor comments (4)
src/rag.rs-351-365 (1)
351-365: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject blank archival inserts in the shared helper.
The HTTP route validates
body.text, but this public helper is also called fromsrc/web/agent/tools.rs:716-753, which forwardscontentdirectly. A whitespace-only tool payload currently still calls the embedding provider and can persist empty archival memory.Suggested fix
) -> Result<crate::models::user_embeddings::UserEmbedding, ApiError> { let user_id = user.uuid; + let text = text.trim(); + if text.is_empty() { + return Err(ApiError::BadRequest); + } let (vector, token_count) = embed_text_via_tinfoil(state, user, auth_method, text).await?;🤖 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 `@src/rag.rs` around lines 351 - 365, The shared helper insert_archival_embedding currently accepts whitespace-only text and proceeds to embed and persist it; add an early validation check in this function so blank or all-whitespace input returns an ApiError before calling embed_text_via_tinfoil. Make sure the rejection applies regardless of whether the call comes from the HTTP route or from the agent tool path in src/web/agent/tools.rs, and keep the check close to the text parameter handling in insert_archival_embedding so the behavior is centralized.src/web/responses/conversations.rs-773-775 (1)
773-775: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDrop the raw conversation trace log.
trace!("Raw conversation object: {:?}", conv)emits the fullConversationpayload, includingmetadata_enc, for every list response. Log the UUID/flags instead.Safer logging
- trace!("Raw conversation object: {:?}", conv); - trace!("Conv metadata_enc present: {}", conv.metadata_enc.is_some()); + trace!( + conversation_uuid = %conv.uuid, + metadata_present = conv.metadata_enc.is_some(), + "Building conversation response" + );🤖 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 `@src/web/responses/conversations.rs` around lines 773 - 775, The `trace!("Raw conversation object: {:?}", conv)` in the conversations response loop is logging the full `Conversation` payload, including sensitive fields like `metadata_enc`; remove that raw object trace and keep only safer diagnostics such as the UUID and relevant boolean flags in the same loop around `conversations_to_return`.src/web/agent/signatures.rs-515-518 (1)
515-518: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize corrected messages before returning them.
The normal parse path strips blank messages, but both correction branches return
output.messagesdirectly. Use the same normalization to avoid persisting/sending empty corrected messages.Proposed fix
Ok(AgentResponseOutput { - messages: output.messages, + messages: normalize_messages(output.messages), reply_reaction: output.reply_reaction, tool_calls: output.tool_calls, }) @@ Ok(AgentResponseOutput { - messages: output.messages, + messages: normalize_messages(output.messages), reply_reaction: String::new(), tool_calls: output.tool_calls, })Also applies to: 555-558
🤖 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 `@src/web/agent/signatures.rs` around lines 515 - 518, The correction return paths in AgentResponseOutput currently pass output.messages through unchanged, which can preserve empty messages unlike the normal parse path. Update the relevant branches in AgentResponseOutput handling within signatures.rs so they apply the same message normalization used by the standard parse flow before returning, ensuring corrected responses do not include blank messages.entrypoint.sh-328-332 (1)
328-332: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake the
/etc/hostsupdate idempotent.These unconditional appends accumulate duplicate aliases whenever the entrypoint is re-run. If the mapped IPs ever change, the stale earlier lines will still win. Replace existing provider entries before writing the new ones.
One way to rewrite the aliases safely
-echo "127.0.0.21 api.push.apple.com" >> /etc/hosts -echo "127.0.0.22 api.sandbox.push.apple.com" >> /etc/hosts -echo "127.0.0.20 fcm.googleapis.com" >> /etc/hosts +sed -i '/ api\.push\.apple\.com$/d;/ api\.sandbox\.push\.apple\.com$/d;/ fcm\.googleapis\.com$/d' /etc/hosts +{ + echo "127.0.0.21 api.push.apple.com" + echo "127.0.0.22 api.sandbox.push.apple.com" + echo "127.0.0.20 fcm.googleapis.com" +} >> /etc/hosts🤖 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 `@entrypoint.sh` around lines 328 - 332, The /etc/hosts update in the entrypoint is not idempotent because the current echo appends in the push-provider block will create duplicate aliases on every run. Update the logic around the APNs/FCM host entries so existing lines for api.push.apple.com, api.sandbox.push.apple.com, and fcm.googleapis.com are removed or replaced before writing the new mappings, while keeping the same log message after the rewrite.
🧹 Nitpick comments (1)
Cargo.toml (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the git dependency to a revision or tag. Cargo.lock currently resolves
dspy-rsto a commit, butbranch = "main"inCargo.tomlstill lets the dependency move whenever the lockfile is refreshed. Preferrev = "..."(or a tag) for a stable manifest.🤖 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 `@Cargo.toml` at line 25, The dspy-rs git dependency is still floating on branch main, so the manifest is not stable even though Cargo.lock currently pins a commit. Update the Cargo.toml entry for dspy-rs to use a fixed rev or a tag instead of branch, keeping the dependency reproducible across lockfile refreshes. Use the existing dspy-rs dependency declaration as the place to make this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fa65ba72-5e66-4671-9800-9a7c230de234
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (56)
Cargo.tomldocs/nitro-deploy.mdentrypoint.shflake.nixmigrations/2026-06-26-224528_agent_background_push_aead/down.sqlmigrations/2026-06-26-224528_agent_background_push_aead/up.sqlsrc/aead_db_tamper_tests.rssrc/agent_background.rssrc/brave.rssrc/db.rssrc/jwt.rssrc/main.rssrc/models/agent_background_grants.rssrc/models/agent_schedule_runs.rssrc/models/agent_schedules.rssrc/models/agents.rssrc/models/conversation_summaries.rssrc/models/memory_blocks.rssrc/models/mod.rssrc/models/notification_deliveries.rssrc/models/notification_events.rssrc/models/project_settings.rssrc/models/push_devices.rssrc/models/responses.rssrc/models/schema.rssrc/models/user_embeddings.rssrc/models/user_preferences.rssrc/push/apns.rssrc/push/binding.rssrc/push/crypto.rssrc/push/fcm.rssrc/push/mod.rssrc/push/worker.rssrc/rag.rssrc/security_invariants.rssrc/seed_wrapping.rssrc/web/agent/compaction.rssrc/web/agent/mod.rssrc/web/agent/reactions.rssrc/web/agent/runtime.rssrc/web/agent/schedules.rssrc/web/agent/signatures.rssrc/web/agent/tools.rssrc/web/agent/vision.rssrc/web/login_routes.rssrc/web/mod.rssrc/web/openai.rssrc/web/platform/common.rssrc/web/platform/mod.rssrc/web/platform/project_routes.rssrc/web/push.rssrc/web/rag.rssrc/web/responses/conversations.rssrc/web/responses/conversions.rssrc/web/responses/handlers.rssrc/web/responses/storage.rs
💤 Files with no reviewable changes (1)
- src/jwt.rs
2 participants
Summary
Testing
Summary by CodeRabbit