Clean rebase agent rewrite onto master#209
Conversation
|
<review_stack_artifact> WalkthroughAdds push notification delivery and project push settings, agent background grants and scheduled wakeups, RAG embeddings/search, agent chat and tool/runtime flows, plus supporting database, schema, and route wiring. ChangesPlatform, build, and schema surface
Push notification delivery
Agent background, runtime, and schedules
RAG storage and search
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
AnthonyRonning
force-pushed
the
codex/agent-aead-clean-rebase
branch
from
ad0c2a7 to
65149ea
Compare
![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.
🟡 Ellipsis incorrectly appended to non-ASCII weather alert descriptions that were not actually truncated
The weather alert description is truncated by character count but the ellipsis decision uses byte length (desc.len() > 200 at src/brave.rs:575), so non-ASCII descriptions shorter than 200 characters but longer than 200 bytes get a spurious "..." suffix.
Impact: Weather alerts containing non-ASCII characters (e.g., accented or CJK text) display a trailing "..." even when the full text is shown.
Byte-length vs character-count mismatch in truncation guard
At src/brave.rs:571, the description is truncated using desc.chars().take(200), which counts Unicode scalar values. But the guard at line 575 uses desc.len(), which counts bytes. For a 150-character string of 3-byte-each CJK characters (450 bytes), chars().take(200) keeps all 150 characters (no truncation), yet desc.len() > 200 evaluates to true (450 > 200), so "..." is appended to text that was never truncated.
The fix is to use desc.chars().count() > 200 instead of desc.len() > 200.
| if desc.len() > 200 { "..." } else { "" } | |
| if desc.chars().count() > 200 { "..." } else { "" } |
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)?; | ||
| } else { | ||
| state | ||
| .db | ||
| .delete_all_conversations(user.uuid) | ||
| .map_err(error_mapping::map_generic_db_error)?; | ||
| } |
There was a problem hiding this comment.
🚩 Delete-all-conversations preserves the main agent conversation but deletes subagent conversations
At src/web/responses/conversations.rs:815-832, when a user calls 'delete all conversations', the code excludes only the main agent's conversation. Subagent conversations are deleted, which will cascade-delete the corresponding agent rows via FK constraints. This means 'delete all conversations' silently destroys all subagents while preserving the main agent. This is arguably correct (subagents are conversation-backed and should go away with their conversations), but could surprise users who expect subagents to survive a conversation purge. The behavior differs from the dedicated DELETE /v1/agent endpoint which explicitly handles subagent cleanup.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (26)
src/rag.rs-625-660 (1)
625-660: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame cursor-advancement bug as
load_all_user_embeddings.
load_user_embeddings_by_tagshas the identical structure:last_id = id(Line 658) runs only for accepted rows, so a fully-skipped batch never advances the cursor and loops indefinitely. Apply the same fix (assignlast_id = idbefore the dim-mismatchcontinue).🤖 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 625 - 660, In load_user_embeddings_by_tags, the pagination cursor can stall because last_id is only updated after a row is accepted into out, so a batch with only dim-mismatched rows will never advance. Move the last_id = id update in the EmbeddingScanRow loop so it happens before the vector length check and the continue path, matching the fix used in load_all_user_embeddings and preventing an infinite retry on skipped rows.src/rag.rs-515-550 (1)
515-550: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPotential infinite loop: pagination cursor not advanced for skipped rows.
last_id = idis assigned only after the dim-mismatchcontinue(Line 548). If every row in a batch is skipped (e.g., corrupted/mismatched rows),last_idis never updated,rows.is_empty()stays false, and the next iteration re-fetches the identical batch forever. Cursor advancement must be independent of whether a row is accepted.Proposed fix: advance cursor before the skip
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; }🤖 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 - 550, The pagination cursor in `load_cached_embeddings` / the row-processing loop is only advanced after a row is accepted, so skipped rows can cause the same batch to be re-fetched forever. Update the `for row in rows` handling so `last_id` is advanced independently of the dim-mismatch check (before any `continue`), ensuring corrupted or skipped `EmbeddingScanRow` entries still move the cursor forward and the loop can terminate.flake.nix-280-284 (1)
280-284: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd the missing
dspy-rs-0.7.3hash
Cargo.lockincludesdspy-rsas a git source, socargoLock.outputHashesneeds adspy-rs-0.7.3entry here as well.🤖 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 `@flake.nix` around lines 280 - 284, Add the missing cargoLock output hash for the git dependency referenced by Cargo.lock: update the outputHashes set in flake.nix to include a dspy-rs-0.7.3 entry alongside the existing baml-ids, minijinja, and rig-core hashes, using the correct hash value so the lockfile-generated build can resolve that source.migrations/2026-06-26-224528_agent_background_push_aead/down.sql-1-2 (1)
1-2: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winScope rollback cleanup to background-agent notifications only.
These deletes wipe all notification events/deliveries, including existing
request_continuationrows. Delete only rows tied tosource_kind = 'agent_background'before restoring the narrower check constraint.🤖 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 1 - 2, The rollback in the migration cleanup is deleting all rows from notification_deliveries and notification_events instead of just the background-agent data. Update the down migration to restrict both deletes to rows associated with source_kind = 'agent_background' so existing request_continuation records are preserved, then proceed with restoring the narrower check constraint.src/web/agent/schedules.rs-1304-1329 (1)
1304-1329: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake message insertion atomic with run-output tracking.
insert_assistant_messagecommits beforerecord_outputverifies the lease. Ifrecord_outputis not applied, the message remains persisted butoutput_count/first_output_atstay unchanged, so recovery can retry and duplicate the assistant output.🤖 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 1304 - 1329, Make the scheduled assistant message write atomic with the run-output update in schedules::run handling. In the loop that calls runtime.insert_assistant_message and then AgentScheduleRun::record_output, ensure the message is not permanently persisted unless the lease check succeeds, by either moving both operations into a single transactional flow or deleting/rolling back the inserted message when write_result.was_applied() is false. Keep the fix centered around insert_assistant_message, record_output, and the persisted_messages push so output_count/first_output_at cannot diverge from stored messages.migrations/2026-06-26-224528_agent_background_push_aead/up.sql-1-13 (1)
1-13: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid wiping schedules to add
description_enc.Lines 1-2 delete every schedule/run before dropping
description. If any environment already has scheduled-agent data, this migration silently loses it. Prefer squashing the unreleased schedule migration, or use a backfill/compatibility migration that preserves existing rows instead of global deletes.🤖 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 1 - 13, The migration currently wipes all rows from agent_schedule_runs and agent_schedules before changing the schema, which will silently destroy existing scheduled-agent data. Update the up.sql for this migration to preserve rows: remove the global DELETEs, and instead use a backfill/compatibility approach around altering agent_schedules and adding description_enc, or squash the unreleased schedule migration so the schema change lands without data loss. Ensure the change still updates user_seed_wrappings and the agent_schedules table safely.src/web/agent/schedules.rs-1050-1067 (1)
1050-1067: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftStop the scheduled turn when heartbeat renewal fails.
heartbeat_schedule_run_leasecan break on renewal failure whilerun_scheduled_agent_turncontinues executing tools/messages. Once the lease expires, another worker can lease the same run, causing duplicate side effects.Also applies to: 1357-1392
🤖 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 1050 - 1067, The scheduled turn flow in run_scheduled_agent_turn must stop as soon as heartbeat_schedule_run_lease stops renewing the lease, because the current tokio::spawn heartbeat can fail while the turn keeps running and causing duplicate side effects. Update the scheduling logic around the heartbeat_handle/stop_tx flow so lease-renewal failure is observed and propagated into the main turn execution, and apply the same fix in the other scheduled-turn path referenced by the comment so both locations abort work when the heartbeat task ends unexpectedly.src/web/agent/schedules.rs-1011-1041 (1)
1011-1041: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMark leased runs failed/retry on grant preflight failures.
These
?/return Errpaths happen after the run is leased but before the retry/failure transition logic. A permanent grant/key/policy mismatch will only log, wait for lease expiry, and be re-leased repeatedly until staleness expires.🤖 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 - 1041, The preflight grant/decryption failures in the schedule execution path currently only return errors after the run has already been leased, which leaves permanent mismatches to be re-leased repeatedly. Update the leased-run handling around decrypt_background_grant_v1, get_user_key_for_agent_background_grant, decrypt_string, and verify_scheduled_policy so these early failure paths transition the run to a failed/retry state instead of just bubbling an error; keep the existing error messages, but ensure the lease-to-state update happens before returning on grant/key/policy preflight failures.src/web/agent/schedules.rs-650-665 (1)
650-665: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRevoke the background grant when cancelling a schedule.
Cancellation leaves
agent_background_grants.revoked_atnull, soget_active_by_schedulecan still return the background capability after the user cancels the schedule. Revoke the active grant in the same transaction as the schedule cancellation.Proposed direction
let cancelled_runs = AgentScheduleRun::cancel_unstarted_for_schedule(conn, schedule.id) .map_err(|e| match e { AgentScheduleRunError::DatabaseError(err) => { AgentScheduleError::DatabaseError(err) } })?; + + diesel::update( + agent_background_grants::table + .filter(agent_background_grants::schedule_id.eq(schedule.id)) + .filter(agent_background_grants::revoked_at.is_null()), + ) + .set(( + agent_background_grants::revoked_at.eq(diesel::dsl::now), + agent_background_grants::updated_at.eq(diesel::dsl::now), + )) + .execute(conn) + .map_err(AgentScheduleError::DatabaseError)?;🤖 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 650 - 665, When cancelling a schedule in the schedule-cancellation flow in AgentSchedule, also revoke the active background grant in the same transaction so agent_background_grants.revoked_at is set and get_active_by_schedule can no longer return it. Update the cancellation path around AgentSchedule::cancel and the existing database update/AgentScheduleRun::cancel_unstarted_for_schedule sequence to call the background-grant revocation logic for the same schedule before committing.src/models/agent_schedule_runs.rs-121-168 (1)
121-168: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject writes after the lease has expired.
Lines 127-160 and the terminal updates only verify
lease_owner. Since expired leases are eligible for another worker, a stale worker can still renew, record output, or complete the run afterlease_expires_at.🔒 Suggested lease guard
WHERE id = $1 AND status = 'leased' AND lease_owner = $2 + AND lease_expires_at > NOW()Apply the same guard to
renew_lease,record_output,mark_retry, and the Diesel terminal transitions:.filter(agent_schedule_runs::status.eq("leased")) .filter(agent_schedule_runs::lease_owner.eq(Some(expected_lease_owner.to_string()))) +.filter(agent_schedule_runs::lease_expires_at.gt(diesel::dsl::now))Also applies to: 171-317
🤖 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 121 - 168, The lease update methods in AgentScheduleRun need to reject stale workers by checking that the lease is still valid, not just that lease_owner matches. Update renew_lease, record_output, mark_retry, and the terminal transition methods in AgentScheduleRunWriteResult to include a lease_expires_at > NOW() guard (or equivalent current-time check) alongside the existing leased/status and lease_owner conditions so expired leases cannot be renewed or written to.src/web/platform/common.rs-107-108 (1)
107-108: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon’t silently default encrypted previews to disabled.
Because
update_push_settingsoverwrites stored settings, omittingencrypted_preview_enabledcurrently persistsfalse. Make the field required for fullPUT, or useOption<bool>and preserve the existing value when omitted.🛡️ Proposed direction
- #[serde(default)] - pub encrypted_preview_enabled: bool, + pub encrypted_preview_enabled: bool,🤖 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/common.rs` around lines 107 - 108, The `encrypted_preview_enabled` field in `UpdatePushSettings` is being silently defaulted to false, which causes `update_push_settings` to overwrite an existing enabled value when the client omits it. Update the `common.rs` settings type so `encrypted_preview_enabled` is either required for the full `PUT` path or modeled as `Option<bool>` and merged in `update_push_settings` to preserve the current stored value when absent.migrations/2026-03-07-120000_push_notifications_v1/up.sql-43-44 (1)
43-44: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAllow all notification source kinds in the DB constraint.
src/push/mod.rsemitsagent_background, but this check only permitsrequest_continuation, so background notifications can fail at insert time unless a later migration drops or widens this constraint.🐛 Proposed fix
- CHECK (source_kind IN ('request_continuation')), + CHECK (source_kind IN ('request_continuation', 'agent_background')),🤖 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-03-07-120000_push_notifications_v1/up.sql` around lines 43 - 44, The DB constraint for source_kind is too narrow and blocks background notifications emitted by src/push/mod.rs. Update the CHECK constraint in the push_notifications migration to allow all current source kinds, including agent_background, and ensure the default/constraint stay aligned with the values produced by the push notification code path.src/push/worker.rs-528-530 (1)
528-530: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep background-grant DB errors retryable.
AgentBackgroundGrant::get_by_iddatabase failures are converted toCryptoError, andclassify_internal_push_errortreats that as permanentFailed. Map the model’s database error back toPushError::DatabaseErrorso transient DB issues retry instead.🤖 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 528 - 530, In the `worker.rs` push flow, `AgentBackgroundGrant::get_by_id` is currently wrapping DB failures as `PushError::CryptoError`, which makes `classify_internal_push_error` treat them as permanent. Update the error mapping in this lookup path to return `PushError::DatabaseError` for the `get_by_id` failure while keeping the missing-grant case as-is, so transient database issues remain retryable.src/push/worker.rs-223-239 (1)
223-239: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRenew the lease while sending to providers.
The provider send happens outside the DB lease, and the lease can expire before
mark_sent/mark_retry. Another worker can then lease the same delivery and send a duplicate push before the first worker records its outcome.🤖 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 - 239, The lease can expire while `build_send_outcome` and provider sending are in progress, allowing another worker to pick up the same delivery and duplicate the push. Update `push::worker::send` to renew or extend the DB lease during the provider-send path before calling `mark_sent` or `mark_retry`, using the existing delivery/state handling around `build_send_outcome` and the `send_outcome` match. Ensure the lease refresh happens while the send is underway so the current worker retains ownership until the outcome is recorded.src/push/mod.rs-311-314 (1)
311-314: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFilter push devices by project before creating deliveries.
list_active_for_usercan include devices from other projects, but this event is project-scoped. Those extra rows inflatedelivery_countand create deliveries the worker later cancels as principal mismatches.Proposed fix
- let devices = PushDevice::list_active_for_user(&mut conn, request.user_id)?; + let devices = PushDevice::list_active_for_user(&mut conn, request.user_id)?; + let devices: Vec<_> = devices + .into_iter() + .filter(|device| device.project_id == request.project_id) + .collect(); if devices.is_empty() { return Ok(None); }🤖 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/mod.rs` around lines 311 - 314, The push delivery setup in push::mod::create delivery flow is using PushDevice::list_active_for_user without narrowing to the current project, so devices from other projects are being counted and scheduled. Update the device selection logic in this path to filter by the event’s project before building deliveries, then keep the empty-devices early return and downstream delivery_count calculation aligned with the filtered set.src/push/apns.rs-112-135 (1)
112-135: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClassify APNs permanent token errors as invalid tokens. APNs
400 BadDeviceTokenand400 DeviceTokenNotForTopicstill fall through toFailed, so those dead tokens stay active and keep getting push attempts; map them toInvalidTokentoo.🤖 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 - 135, The APNs status handling in the push outcome mapping is missing permanent token failures, so 400 BadDeviceToken and 400 DeviceTokenNotForTopic still fall through to PushSendOutcome::Failed. Update the match in apns.rs alongside the existing 410 InvalidToken branch so these 400 token-related errors are also classified as PushSendOutcome::InvalidToken, preserving the provider status code and using the same error fallback behavior.src/push/binding.rs-181-182 (1)
181-182: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winEnforce row/event binding inside decrypt helpers.
The decryptors currently return plaintext without applying the existing
matches_device_row/matches_eventchecks. That leaves callers able to use stale or mismatched capability/payload bytes if they forget the manual validation.Proposed fix
- let bytes = decrypt_aead_v1(&key, encrypted, &aad)?; - serde_json::from_slice(&bytes).map_err(|_| EncryptError::BadData) + let bytes = decrypt_aead_v1(&key, encrypted, &aad)?; + let plaintext: PushDeviceCapabilityPlaintextV1 = + serde_json::from_slice(&bytes).map_err(|_| EncryptError::BadData)?; + if !plaintext.matches_device_row(device) { + return Err(EncryptError::BadData); + } + Ok(plaintext) @@ - let bytes = decrypt_aead_v1(&key, encrypted, &aad)?; - serde_json::from_slice(&bytes).map_err(|_| EncryptError::BadData) + let bytes = decrypt_aead_v1(&key, encrypted, &aad)?; + let plaintext: NotificationPreviewPayloadV1 = + serde_json::from_slice(&bytes).map_err(|_| EncryptError::BadData)?; + if !plaintext.matches_event(event, background_grant_uuid) { + return Err(EncryptError::BadData); + } + Ok(plaintext)Also applies to: 221-222
🤖 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 181 - 182, The decrypt helpers are returning plaintext without enforcing the existing binding checks, so stale or mismatched payloads can slip through. Update the decrypt flow in the relevant decryptor(s) in binding.rs so that after `decrypt_aead_v1` and JSON decoding, the result is validated with the appropriate `matches_device_row` or `matches_event` helper before being returned. Make this enforcement part of the helper itself so callers of the decrypt functions cannot bypass the row/event binding verification.src/push/fcm.rs-174-199 (1)
174-199: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not POST OAuth assertions to arbitrary
token_urivalues.
token_uricomes from the stored service-account JSON, so a project-controlled secret can make the worker issue outbound POSTs to non-Google URLs. Pin this to the expected Google token endpoint or a strict HTTPS allowlist.Proposed fix
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 OAuth token exchange in the FCM flow is using the service account’s token_uri directly, which allows posting assertions to arbitrary URLs. Update the token request logic in the Fcm JWT exchange path to ignore untrusted token_uri values and instead use the fixed Google token endpoint or a strict HTTPS allowlist before calling transport.client.post. Keep the change localized around the token_uri selection and the request construction so the rest of the signing flow remains unchanged.src/web/agent/tools.rs-590-590 (1)
590-590: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winClamp retrieval limits before running semantic search.
limit/top_kcome from model-supplied tool args and can be arbitrarily large, producing expensive searches and oversized tool outputs. Cap them to a small maximum, similar to Brave search.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().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().ok()) + .unwrap_or(5) + .clamp(1, 20);Also applies to: 790-790
🤖 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` at line 590, The semantic search retrieval args are taking model-supplied values directly, so clamp `limit` and `top_k` to a small fixed maximum before use. Update the parsing sites in `tools.rs` where `limit` is read from `args` and the corresponding `top_k` path so both enforce the cap consistently, preventing oversized searches and tool outputs.src/web/agent/tools.rs-276-308 (1)
276-308: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake memory updates atomic to prevent lost writes.
These tools read/decrypt/modify/write memory outside a transaction or optimistic compare. Concurrent live/background agent runs can overwrite each other’s appended or corrected memory.
Also applies to: 361-391, 445-471
🤖 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 276 - 308, Make the memory edit path atomic to avoid lost writes during concurrent agent runs: in the block update flow around MemoryBlock::get_by_user_and_label, decrypt_string, and update_block_value, stop doing a separate read/modify/write without coordination. Move the load, validation, replacement, and save into a single transaction or add an optimistic concurrency check (for example, compare the stored value/version before writing and retry or fail on mismatch). Apply the same atomic pattern to the other memory-update paths noted in the diff so appended/corrected content cannot overwrite concurrent changes.src/web/agent/signatures.rs-409-417 (1)
409-417: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove decrypted prompt bodies from trace logs.
system_promptanduser_promptinclude user memory, conversation history, and current input. Logging them verbatim can leak decrypted user data whenever trace logging is enabled; keep lengths/metadata only or gate full prompts behind a local-only debug feature.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 helper is logging decrypted prompt bodies verbatim via system_prompt and user_prompt, which can expose sensitive user data when trace logging is enabled. Update trace_generated_prompt to keep only safe metadata such as prompt_kind and the prompt lengths, and remove the full prompt fields from the trace output unless they are explicitly gated behind a local-only debug feature or equivalent safeguard.src/web/agent/tools.rs-299-306 (1)
299-306: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAvoid replacing every matching memory occurrence.
String::replaceupdates all occurrences ofold; if the same phrase appears in multiple facts, one correction can unintentionally rewrite unrelated memory entries. Usereplacen(..., 1)or reject ambiguous multiple matches.Proposed fix
- let new_value = value.replace(old, new); + if value.matches(old).nth(1).is_some() { + return ToolResult::error( + "Old content appears multiple times; provide a more specific exact match.", + ); + } + + let new_value = value.replacen(old, new, 1);🤖 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 299 - 306, The replacement logic in the memory update flow currently uses String::replace, which will rewrite every matching occurrence of old in the block. Update the code in the tool handling path around the old/new substitution so it only changes a single intended match, using replacen(..., 1) or by detecting multiple matches and returning an error. Keep the existing validation in the memory block handling code and apply the fix where new_value is constructed.src/web/agent/runtime.rs-378-429 (1)
378-429: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake onboarding seeding concurrency-safe.
The count-then-insert guard is not atomic. Two concurrent
/v1/agent/initcalls can both observe zero messages and each insert the three onboarding assistant messages. Wrap the count/insert in a transaction and lock the conversation row, or add an idempotency marker/constraint for seeded onboarding.🤖 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 378 - 429, The onboarding seeding logic in the seeding function is vulnerable to a race because it checks message counts and then inserts outside an atomic boundary. Update the flow around the user_messages/assistant_messages count checks and the onboarding_message_texts insert loop to run inside a transaction, and lock the relevant conversation row (or otherwise enforce a unique idempotency marker/constraint) so concurrent /v1/agent/init calls cannot seed twice. Keep the existing error handling/logging in the count and insert paths, but ensure they operate within the same transactional scope.src/web/agent/mod.rs-545-552 (1)
545-552: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a bodyless decrypt layer for reaction DELETE routes.
The
.layer(decrypt_request::<SetMessageReactionRequest>)is applied to both POST and DELETE on the reaction MethodRouter. The clear handlers do not readSetMessageReactionRequest, so normal DELETE requests without an emoji body can fail in middleware before reaching the handler.Proposed split by method
.route( - "/v1/agent/items/:item_id/reaction", + "/v1/agent/items/{item_id}/reaction", post(set_main_agent_item_reaction) - .delete(clear_main_agent_item_reaction) .layer(from_fn_with_state( app_state.clone(), decrypt_request::<SetMessageReactionRequest>, )), ) + .route( + "/v1/agent/items/{item_id}/reaction", + delete(clear_main_agent_item_reaction) + .layer(from_fn_with_state(app_state.clone(), decrypt_request::<()>)), + ) @@ .route( - "/v1/agent/subagents/:id/items/:item_id/reaction", + "/v1/agent/subagents/{id}/items/{item_id}/reaction", post(set_subagent_item_reaction) - .delete(clear_subagent_item_reaction) .layer(from_fn_with_state( app_state.clone(), decrypt_request::<SetMessageReactionRequest>, )), ) + .route( + "/v1/agent/subagents/{id}/items/{item_id}/reaction", + delete(clear_subagent_item_reaction) + .layer(from_fn_with_state(app_state.clone(), decrypt_request::<()>)), + )Also applies to: 594-600
🤖 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 545 - 552, The reaction MethodRouter currently applies decrypt_request::<SetMessageReactionRequest> to both set_main_agent_item_reaction and clear_main_agent_item_reaction, so DELETE requests can fail before reaching the clear handler. Split the middleware by method in the router that defines "/v1/agent/items/:item_id/reaction" (and the similar reaction route near the other referenced location): keep the decrypt layer only on the POST path that needs SetMessageReactionRequest, and use a bodyless or no decrypt layer for the DELETE path handled by clear_main_agent_item_reaction.src/web/agent/runtime.rs-1730-1880 (1)
1730-1880: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon’t hold a DB connection across LLM and embedding awaits.
Line 1731 checks out a pool connection and keeps it alive through
summarize(...).awaitandget_embedding_vector(...).await. During compaction, slow provider calls can pin scarce DB connections and degrade unrelated requests. Load the rows, drop/release the connection before external awaits, then reacquire for the summary insert.Sketch of the connection-lifetime fix
let raw_messages = RawThreadMessage::get_messages_by_ids(&mut conn, self.conversation.id, &to_summarize) .map_err(|e| { error!("Failed to load messages for compaction: {e:?}"); ApiError::InternalServerError })?; if raw_messages.is_empty() { return Ok(()); } + + drop(conn); let mut formatted: Vec<String> = Vec::new(); for m in &raw_messages { if m.message_type == "reasoning" { continue; @@ let new_summary = NewConversationSummary { uuid: Uuid::new_v4(), user_id: self.user.uuid, conversation_id: self.conversation.id, @@ previous_summary_id, }; + let mut conn = self + .state + .db + .get_pool() + .get() + .map_err(|_| ApiError::InternalServerError)?; let _ = new_summary.insert(&mut conn).map_err(|e| { error!("Failed to insert conversation summary: {e:?}"); 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/agent/runtime.rs` around lines 1730 - 1880, The maybe_compact method is holding a database connection from get_pool().get() across the slow summarize and get_embedding_vector awaits, which can pin scarce pool connections. Refactor maybe_compact so it only uses conn to load the summary, metadata, and raw_messages, then explicitly release/drop it before calling self.compaction.summarize and crate::web::get_embedding_vector. Reacquire a fresh connection only when inserting NewConversationSummary, keeping the connection lifetime as short as possible.src/web/agent/runtime.rs-835-844 (1)
835-844: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject or resolve
file_idimage parts before preparing the turn.Line 835 only handles
image_url, butsrc/web/agent/mod.rstreats non-emptyfile_idimage parts as non-empty input. Afile_id-only message reaches the SSE worker and fails later as a generic prepare error, whiletext + file_idsilently drops the image.Proposed guard if file-backed images are not supported yet
let image_url = match &normalized { MessageContent::Parts(parts) => parts.iter().find_map(|p| match p { MessageContentPart::InputImage { image_url: Some(url), .. } => Some(url.clone()), _ => None, }), MessageContent::Text(_) => None, }; + + let has_unsupported_file_attachment = match &normalized { + MessageContent::Parts(parts) => parts.iter().any(|p| { + matches!( + p, + MessageContentPart::InputImage { + file_id: Some(file_id), + .. + } if !file_id.trim().is_empty() + ) + }), + MessageContent::Text(_) => false, + }; + if has_unsupported_file_attachment { + return Err(ApiError::BadRequest); + }🤖 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 835 - 844, The turn preparation in `runtime.rs` only inspects `image_url`, so `file_id`-backed image parts are either silently ignored or fail later as a generic prepare error. Update the normalization/validation path around `MessageContent::Parts`, `MessageContentPart::InputImage`, and the turn-prep logic in `src/web/agent/runtime.rs` (and keep it aligned with `src/web/agent/mod.rs`) to explicitly detect non-empty `file_id` image parts before starting the SSE worker. If file-backed images are unsupported, reject them early with a clear error; otherwise, resolve them into a usable image source so `text + file_id` does not drop the image.
🟡 Minor comments (4)
src/models/user_preferences.rs-103-115 (1)
103-115: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
updated_atis not bumped on conflict update.Same issue as
memory_blocks::insert_or_update: the upsert sets onlyvalue_enc, leavingupdated_atunchanged on edits unless a DB trigger maintains it. See the verification request onsrc/models/memory_blocks.rs.Proposed fix (if no DB trigger maintains updated_at)
.do_update() - .set(user_preferences::value_enc.eq(self.value_enc.clone())) + .set(( + user_preferences::value_enc.eq(self.value_enc.clone()), + user_preferences::updated_at.eq(diesel::dsl::now), + )) .get_result::<UserPreference>(conn)🤖 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/user_preferences.rs` around lines 103 - 115, `UserPreference::insert_or_update` only updates `value_enc` on conflict, so `updated_at` is not refreshed when an existing preference is edited. Update the `do_update().set(...)` clause in `insert_or_update` to also bump `updated_at` on conflict, matching the behavior expected in `memory_blocks::insert_or_update`, unless a DB trigger already handles it.src/models/agent_schedules.rs-1-7 (1)
1-7: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject duplicate weekdays in weekly specs.
Lines 155-161 accept values like
[monday, monday]; the spec is persisted as-is. Validate uniqueness here so downstream scheduling does not have to deduplicate defensively.✅ Suggested validation
use serde_json::Value; +use std::collections::HashSet; use thiserror::Error; use uuid::Uuid; -#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq, Hash)] #[serde(rename_all = "snake_case")] pub enum ScheduleWeekday {if weekdays.is_empty() { return Err(AgentScheduleError::InvalidSpec( "weekly schedules require at least one weekday".to_string(), )); } + + let unique_weekdays: HashSet<_> = weekdays.iter().collect(); + if unique_weekdays.len() != weekdays.len() { + return Err(AgentScheduleError::InvalidSpec( + "weekly schedules cannot repeat weekdays".to_string(), + )); + } Ok(())Also applies to: 31-41, 150-161
🤖 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_schedules.rs` around lines 1 - 7, Reject duplicate weekdays in the weekly schedule spec by adding uniqueness validation in the agent_schedules model’s weekly-spec parsing/validation path, where weekday lists are currently accepted and persisted as-is. Update the relevant constructor/deserializer/validator in agent_schedules.rs so repeated days like the same weekday appearing twice are rejected before save, and make sure the check runs alongside the existing weekly spec validation logic.src/models/agents.rs-114-150 (1)
114-150: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle missing pagination cursors explicitly.
If
after_uuidis stale or filtered out, Lines 132-150 skip cursor filtering and return the first page again. Return an empty page or a cursor error instead.🔁 Suggested cursor handling
- if let Some((updated_at, id)) = cursor_subagent { - if order == "desc" { - query = query.filter( - conversations::updated_at - .lt(updated_at) - .or(conversations::updated_at - .eq(updated_at) - .and(agents::id.lt(id))), - ); - } else { - query = query.filter( - conversations::updated_at - .gt(updated_at) - .or(conversations::updated_at - .eq(updated_at) - .and(agents::id.gt(id))), - ); - } + let Some((updated_at, id)) = cursor_subagent else { + return Ok(Vec::new()); + }; + + if order == "desc" { + query = query.filter( + conversations::updated_at + .lt(updated_at) + .or(conversations::updated_at + .eq(updated_at) + .and(agents::id.lt(id))), + ); + } else { + query = query.filter( + conversations::updated_at + .gt(updated_at) + .or(conversations::updated_at + .eq(updated_at) + .and(agents::id.gt(id))), + ); }🤖 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/agents.rs` around lines 114 - 150, The pagination cursor handling in the agents query should not silently fall back to the first page when after_uuid cannot be found or is excluded by created_by_filter. In the cursor lookup path inside the agents list query, detect when cursor_subagent is None and return an explicit empty page or a cursor-related AgentError instead of continuing without cursor filtering. Keep the existing ordering logic for the valid cursor case, but make the missing-cursor behavior explicit so stale cursors do not produce duplicate results.src/web/agent/tools.rs-721-726 (1)
721-726: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize archival tags consistently on insert and search.
Search lowercases tags, but insert stores the original casing. If tag filtering is case-sensitive, tags like
Travelwon’t match a later search fortravel.Proposed fix
- .map(|s| s.trim().to_string()) + .map(|s| s.trim().to_lowercase())Also applies to: 792-800
🤖 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 721 - 726, Normalize tag values consistently in the tag parsing flow so insert and search use the same casing. Update the tag handling in the relevant tools code around the tags extraction logic and the matching search/filter path so tags are stored and compared in a single normalized form, using the existing tag parsing symbols in this area to keep `Travel` and `travel` equivalent.
🧹 Nitpick comments (4)
Cargo.toml (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGit dependency tracks a moving branch.
dspy-rspoints atbranch = "main"with norev/tag. The exact commit is pinned byCargo.lock, so builds remain reproducible, but anycargo update -p dspy-rswill silently advance to whatevermainis at that moment. Consider pinning arev(ortag) for an auditable supply chain, especially since this crate feeds the agent runtime.Note: a git dependency also needs a matching
cargoLock.outputHashesentry inflake.nix— verified separately in that file.🤖 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 dependency is tracking a moving git branch without an explicit commit or tag, so update the Cargo.toml entry to pin it to a fixed rev or tag instead of branch = "main". Use the dspy-rs dependency declaration as the target for the change, and keep the version source auditable and stable for future cargo update -p dspy-rs runs.src/main.rs (1)
1234-1270: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAgent-background seed wrapping skips the round-trip verification the other credential kinds perform.
new_password_seed_wrapping_for_userandnew_oauth_seed_wrapping_for_userboth decrypt the freshly-built wrapping and assert it round-trips toplaintext_seedbefore returning (viaverify_new_*).new_agent_background_seed_wrapping_for_userreturns the wrapping without that check, so an encryption/binding regression here would only surface later at grant-use time rather than at creation. Consider adding the same verify-before-return guard for consistency and earlier failure detection.🤖 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/main.rs` around lines 1234 - 1270, The agent-background seed wrapping path in new_agent_background_seed_wrapping_for_user is missing the same round-trip verification used by new_password_seed_wrapping_for_user and new_oauth_seed_wrapping_for_user. After creating the NewUserSeedWrapping, add the same verify-before-return guard pattern (via the corresponding verify_new_* helper or equivalent decrypt-and-compare check) so the freshly encrypted seed is validated against plaintext_seed before returning, with errors propagated as EncryptionError like the other credential flows.docs/nitro-deploy.md (1)
605-605: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor markdown lint: heading level jump and unlabeled fences.
The
#### APNs Productionheadings sit directly under the##section (MD001 expects###), and the fenced blocks omit a language (MD040). Purely cosmetic, but easy to clean up: use###for these subsection headings and addbash/ini/yamlinfo strings to the code fences.🤖 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 `@docs/nitro-deploy.md` at line 605, The markdown in this section has a heading level jump and unlabeled fenced blocks. Update the APNs subsection headings to match the surrounding structure using the correct `###` level, and add appropriate language identifiers to each fenced block in the doc (such as bash, ini, or yaml) so the markdown lint rules are satisfied. Use the `APNs Production` subsection and nearby fenced examples as the places to update.Source: Linters/SAST tools
src/models/push_devices.rs (1)
22-44: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid serializing the sensitive persistence model by default.
PushDevicecontains encrypted capability material and token/key hashes. Prefer removingSerialize/Deserializehere and using an explicit redacted response DTO for any API output.🛡️ Proposed change
-#[derive(Queryable, Identifiable, Clone, Debug, Serialize, Deserialize)] +#[derive(Queryable, Identifiable, Clone, Debug)]🤖 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 22 - 44, `PushDevice` is currently exposed as a serializable API type even though it contains sensitive token/hash and encrypted capability fields. Remove `Serialize`/`Deserialize` from the `PushDevice` model, keep it as a persistence-only Diesel entity, and introduce a separate redacted response DTO for any outward-facing payloads. Update the code paths that return or serialize `PushDevice` to use that DTO instead of the model.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4afc9386-13ff-4a36-b487-52e361e97eef
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (57)
Cargo.tomldocs/nitro-deploy.mdentrypoint.shflake.nixmigrations/2026-03-07-120000_push_notifications_v1/down.sqlmigrations/2026-03-07-120000_push_notifications_v1/up.sqlmigrations/2026-06-26-224528_agent_background_push_aead/down.sqlmigrations/2026-06-26-224528_agent_background_push_aead/up.sqlsrc/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
| "/v1/agent/items/:item_id", | ||
| get(get_main_agent_item) | ||
| .layer(from_fn_with_state(app_state.clone(), decrypt_request::<()>)), | ||
| ) | ||
| .route( | ||
| "/v1/agent/items/:item_id/reaction", | ||
| post(set_main_agent_item_reaction) | ||
| .delete(clear_main_agent_item_reaction) | ||
| .layer(from_fn_with_state( | ||
| app_state.clone(), | ||
| decrypt_request::<SetMessageReactionRequest>, | ||
| )), | ||
| ) | ||
| .route( | ||
| "/v1/agent/chat", | ||
| post(chat_main).layer(from_fn_with_state( | ||
| app_state.clone(), | ||
| decrypt_request::<AgentChatRequest>, | ||
| )), | ||
| ) | ||
| .route( | ||
| "/v1/agent/subagents", | ||
| get(list_subagents).layer(from_fn_with_state(app_state.clone(), decrypt_request::<()>)), | ||
| ) | ||
| .route( | ||
| "/v1/agent/subagents", | ||
| post(create_subagent).layer(from_fn_with_state( | ||
| app_state.clone(), | ||
| decrypt_request::<CreateSubagentRequest>, | ||
| )), | ||
| ) | ||
| .route( | ||
| "/v1/agent/subagents/:id", | ||
| get(get_subagent).layer(from_fn_with_state(app_state.clone(), decrypt_request::<()>)), | ||
| ) | ||
| .route( | ||
| "/v1/agent/subagents/:id/chat", | ||
| post(chat_subagent).layer(from_fn_with_state( | ||
| app_state.clone(), | ||
| decrypt_request::<AgentChatRequest>, | ||
| )), | ||
| ) | ||
| .route( | ||
| "/v1/agent/subagents/:id/items", | ||
| get(list_subagent_items) | ||
| .layer(from_fn_with_state(app_state.clone(), decrypt_request::<()>)), | ||
| ) | ||
| .route( | ||
| "/v1/agent/subagents/:id/items/:item_id", | ||
| get(get_subagent_item) | ||
| .layer(from_fn_with_state(app_state.clone(), decrypt_request::<()>)), | ||
| ) | ||
| .route( | ||
| "/v1/agent/subagents/:id/items/:item_id/reaction", | ||
| post(set_subagent_item_reaction) | ||
| .delete(clear_subagent_item_reaction) | ||
| .layer(from_fn_with_state( | ||
| app_state.clone(), | ||
| decrypt_request::<SetMessageReactionRequest>, | ||
| )), | ||
| ) | ||
| .route( | ||
| "/v1/agent/subagents/:id", | ||
| delete(delete_subagent) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify legacy Axum route captures added by this PR.
rg -n 'route\(\s*"[^"]*/:[^"]*"' srcRepository: OpenSecretCloud/opensecret
Length of output: 257
Update these routes to Axum 0.7 {param} syntax.
Replace each :item_id / :id capture in this block; Axum 0.7 rejects the legacy form, so the agent router won't build.
🤖 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 541 - 604, The agent routes in mod.rs are
still using the legacy ":param" path capture syntax, which Axum 0.7 no longer
accepts. Update every route pattern in this block to the new "{param}" form for
item_id and id, including the routes wired to get_main_agent_item,
set_main_agent_item_reaction, chat_main, list_subagents, create_subagent,
get_subagent, chat_subagent, list_subagent_items, get_subagent_item,
set_subagent_item_reaction, and delete_subagent, so the router compiles and
matches correctly.
![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!( |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
entrypoint.sh (1)
328-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the
/etc/hostsupdates idempotent.Lines 329-331 append the same mappings every time the entrypoint runs. If this script is re-executed, stale earlier entries stay ahead of any updated mapping and resolution becomes order-dependent. Please upsert or
grep -qFbefore appending; that also lets you collapse the writes into a single redirect.Proposed change
+append_host() { + local entry="$1" + grep -qxF "$entry" /etc/hosts || echo "$entry" +} + -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 +{ + append_host "127.0.0.21 api.push.apple.com" + append_host "127.0.0.22 api.sandbox.push.apple.com" + append_host "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` updates in the entrypoint are not idempotent because the APNs/FCM host mappings are appended every run. Update the host-entry logic in entrypoint.sh to check for each hostname before writing, or use an upsert-style approach so repeated executions do not create duplicate stale entries. Keep the fix localized to the existing APNs/FCM host mapping block and preserve the current log message.Source: Linters/SAST tools
docs/nitro-deploy.md (1)
594-680: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the markdownlint warnings in this section.
This block still trips MD040 on every fence and MD001 at Line 605. Add explicit fence languages (
sh,yaml,ini) and bump the provider subsections to###so the heading levels stay consistent.🤖 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 `@docs/nitro-deploy.md` around lines 594 - 680, This section has markdownlint MD040 on the code fences and MD001 from inconsistent heading nesting. Update the fenced examples in the APNs Production, APNs Sandbox, and FCM subsections to include explicit languages such as sh, yaml, and ini, and change those provider subsection headings to ### so they sit consistently under the surrounding structure. Keep the existing content and service names intact while adjusting only the markdown formatting in this block.Source: Linters/SAST tools
🤖 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 `@src/push/fcm.rs`:
- Around line 174-199: The OAuth token exchange in fcm.rs currently trusts
service_account.token_uri, which allows a project-scoped secret to redirect the
signed JWT assertion to an arbitrary endpoint. Update the token request flow in
the FcmJwtClaims / send-token logic to ignore the secret-provided token_uri and
always use Google’s fixed OAuth token endpoint, or enforce a strict allowlist
that only permits the canonical Google URL before calling transport.client.post.
- Around line 115-135: Treat FCM 403 permission/configuration responses as
permanent failures instead of retryable in the fcm.rs push outcome logic. Update
the status handling around invalidate_fcm_cache and the PushSendOutcome
selection so only 401/UNAUTHENTICATED remains retryable, while 403
PERMISSION_DENIED returns a Failed outcome with the existing error details. Keep
the existing UNREGISTERED/invalid token path unchanged and use the current
status/error variables in the decision logic.
---
Nitpick comments:
In `@docs/nitro-deploy.md`:
- Around line 594-680: This section has markdownlint MD040 on the code fences
and MD001 from inconsistent heading nesting. Update the fenced examples in the
APNs Production, APNs Sandbox, and FCM subsections to include explicit languages
such as sh, yaml, and ini, and change those provider subsection headings to ###
so they sit consistently under the surrounding structure. Keep the existing
content and service names intact while adjusting only the markdown formatting in
this block.
In `@entrypoint.sh`:
- Around line 328-332: The `/etc/hosts` updates in the entrypoint are not
idempotent because the APNs/FCM host mappings are appended every run. Update the
host-entry logic in entrypoint.sh to check for each hostname before writing, or
use an upsert-style approach so repeated executions do not create duplicate
stale entries. Keep the fix localized to the existing APNs/FCM host mapping
block and preserve the current log message.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 98510a9e-2abc-454e-a647-ac20c3477a1e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (58)
Cargo.tomldocs/nitro-deploy.mdentrypoint.shflake.nixmigrations/2026-03-07-120000_push_notifications_v1/down.sqlmigrations/2026-03-07-120000_push_notifications_v1/up.sqlmigrations/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
✅ Files skipped from review due to trivial changes (2)
- src/web/responses/storage.rs
- src/web/platform/mod.rs
🚧 Files skipped from review as they are similar to previous changes (50)
- migrations/2026-03-07-120000_push_notifications_v1/down.sql
- src/security_invariants.rs
- src/web/responses/conversions.rs
- migrations/2026-06-26-224528_agent_background_push_aead/down.sql
- src/models/agent_background_grants.rs
- flake.nix
- src/web/responses/handlers.rs
- src/web/agent/vision.rs
- src/push/apns.rs
- src/models/mod.rs
- src/models/conversation_summaries.rs
- src/agent_background.rs
- migrations/2026-06-26-224528_agent_background_push_aead/up.sql
- src/web/agent/reactions.rs
- Cargo.toml
- src/models/memory_blocks.rs
- src/web/login_routes.rs
- src/models/schema.rs
- src/models/user_embeddings.rs
- src/models/push_devices.rs
- src/models/agents.rs
- src/push/mod.rs
- src/push/worker.rs
- src/web/platform/project_routes.rs
- src/web/mod.rs
- src/web/platform/common.rs
- src/web/openai.rs
- src/models/project_settings.rs
- src/models/notification_events.rs
- src/models/user_preferences.rs
- src/web/rag.rs
- src/web/agent/mod.rs
- migrations/2026-03-07-120000_push_notifications_v1/up.sql
- src/web/agent/compaction.rs
- src/web/push.rs
- src/models/notification_deliveries.rs
- src/seed_wrapping.rs
- src/web/responses/conversations.rs
- src/models/agent_schedules.rs
- src/db.rs
- src/models/agent_schedule_runs.rs
- src/push/binding.rs
- src/main.rs
- src/rag.rs
- src/web/agent/signatures.rs
- src/web/agent/tools.rs
- src/web/agent/runtime.rs
- src/web/agent/schedules.rs
- src/models/responses.rs
- src/brave.rs
| if matches!(status.as_u16(), 401 | 403) { | ||
| invalidate_fcm_cache(transport, event.project_id, &service_account.client_email).await; | ||
| } | ||
|
|
||
| let outcome = if matches!(fcm_error_code.as_deref(), Some("UNREGISTERED")) | ||
| || is_invalid_registration(&error_status, &error_message) | ||
| { | ||
| PushSendOutcome::InvalidToken { | ||
| provider_status_code: Some(status_code), | ||
| error: if let Some(code) = fcm_error_code { | ||
| format!("{}: {}", code, error_message) | ||
| } else { | ||
| error_message | ||
| }, | ||
| } | ||
| } else if matches!(status.as_u16(), 401 | 403 | 429 | 500 | 503) { | ||
| PushSendOutcome::Retryable { | ||
| provider_status_code: Some(status_code), | ||
| error: error_message, | ||
| } | ||
| } else { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Treat FCM 403 permission errors as permanent failures.
Lines 115-135 mark every 403 as Retryable after cache invalidation. FCM uses 403 PERMISSION_DENIED for bad project/service-account bindings and disabled APIs, so these notifications will churn forever instead of surfacing a permanent configuration error. Retry only 401/UNAUTHENTICATED; return Failed for permission/configuration errors.
Proposed change
- if matches!(status.as_u16(), 401 | 403) {
+ if status.as_u16() == 401 || error_status == "UNAUTHENTICATED" {
invalidate_fcm_cache(transport, event.project_id, &service_account.client_email).await;
}
@@
- } else if matches!(status.as_u16(), 401 | 403 | 429 | 500 | 503) {
+ } else if status.as_u16() == 401
+ || error_status == "UNAUTHENTICATED"
+ || matches!(status.as_u16(), 429 | 500 | 503)
+ {
PushSendOutcome::Retryable {
provider_status_code: Some(status_code),
error: error_message,
}📝 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.
| if matches!(status.as_u16(), 401 | 403) { | |
| invalidate_fcm_cache(transport, event.project_id, &service_account.client_email).await; | |
| } | |
| let outcome = if matches!(fcm_error_code.as_deref(), Some("UNREGISTERED")) | |
| || is_invalid_registration(&error_status, &error_message) | |
| { | |
| PushSendOutcome::InvalidToken { | |
| provider_status_code: Some(status_code), | |
| error: if let Some(code) = fcm_error_code { | |
| format!("{}: {}", code, error_message) | |
| } else { | |
| error_message | |
| }, | |
| } | |
| } else if matches!(status.as_u16(), 401 | 403 | 429 | 500 | 503) { | |
| PushSendOutcome::Retryable { | |
| provider_status_code: Some(status_code), | |
| error: error_message, | |
| } | |
| } else { | |
| if status.as_u16() == 401 || error_status == "UNAUTHENTICATED" { | |
| invalidate_fcm_cache(transport, event.project_id, &service_account.client_email).await; | |
| } | |
| let outcome = if matches!(fcm_error_code.as_deref(), Some("UNREGISTERED")) | |
| || is_invalid_registration(&error_status, &error_message) | |
| { | |
| PushSendOutcome::InvalidToken { | |
| provider_status_code: Some(status_code), | |
| error: if let Some(code) = fcm_error_code { | |
| format!("{}: {}", code, error_message) | |
| } else { | |
| error_message | |
| }, | |
| } | |
| } else if status.as_u16() == 401 | |
| || error_status == "UNAUTHENTICATED" | |
| || matches!(status.as_u16(), 429 | 500 | 503) | |
| { | |
| PushSendOutcome::Retryable { | |
| provider_status_code: Some(status_code), | |
| error: error_message, | |
| } | |
| } else { |
🤖 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 115 - 135, Treat FCM 403
permission/configuration responses as permanent failures instead of retryable in
the fcm.rs push outcome logic. Update the status handling around
invalidate_fcm_cache and the PushSendOutcome selection so only
401/UNAUTHENTICATED remains retryable, while 403 PERMISSION_DENIED returns a
Failed outcome with the existing error details. Keep the existing
UNREGISTERED/invalid token path unchanged and use the current status/error
variables in the decision logic.
| let token_uri = service_account | ||
| .token_uri | ||
| .as_deref() | ||
| .unwrap_or(DEFAULT_GOOGLE_TOKEN_URI); | ||
| let now = Utc::now().timestamp(); | ||
| let claims = FcmJwtClaims { | ||
| iss: &service_account.client_email, | ||
| scope: GOOGLE_TOKEN_SCOPE, | ||
| aud: token_uri, | ||
| exp: now + 3600, | ||
| iat: now - 30, | ||
| }; | ||
| let header = Header::new(Algorithm::RS256); | ||
| let signing_key = EncodingKey::from_rsa_pem(service_account.private_key.as_bytes()) | ||
| .map_err(|e| PushError::InvalidSecret(e.to_string()))?; | ||
| let assertion = encode(&header, &claims, &signing_key)?; | ||
|
|
||
| let response = transport | ||
| .client | ||
| .post(token_uri) | ||
| .form(&[ | ||
| ("grant_type", "urn:ietf:params:oauth:grant-type:jwt-bearer"), | ||
| ("assertion", assertion.as_str()), | ||
| ]) | ||
| .send() | ||
| .await?; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin the OAuth token endpoint to Google.
Lines 174-199 trust token_uri from the project-scoped service-account JSON and then POST the signed JWT assertion there. That turns tenant configuration into a blind SSRF target and leaks the assertion to any attacker-controlled URL. Please ignore token_uri from the secret or strictly allowlist https://oauth2.googleapis.com/token.
Proposed change
- let token_uri = service_account
- .token_uri
- .as_deref()
- .unwrap_or(DEFAULT_GOOGLE_TOKEN_URI);
+ let token_uri = DEFAULT_GOOGLE_TOKEN_URI;📝 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.
| let token_uri = service_account | |
| .token_uri | |
| .as_deref() | |
| .unwrap_or(DEFAULT_GOOGLE_TOKEN_URI); | |
| let now = Utc::now().timestamp(); | |
| let claims = FcmJwtClaims { | |
| iss: &service_account.client_email, | |
| scope: GOOGLE_TOKEN_SCOPE, | |
| aud: token_uri, | |
| exp: now + 3600, | |
| iat: now - 30, | |
| }; | |
| let header = Header::new(Algorithm::RS256); | |
| let signing_key = EncodingKey::from_rsa_pem(service_account.private_key.as_bytes()) | |
| .map_err(|e| PushError::InvalidSecret(e.to_string()))?; | |
| let assertion = encode(&header, &claims, &signing_key)?; | |
| let response = transport | |
| .client | |
| .post(token_uri) | |
| .form(&[ | |
| ("grant_type", "urn:ietf:params:oauth:grant-type:jwt-bearer"), | |
| ("assertion", assertion.as_str()), | |
| ]) | |
| .send() | |
| .await?; | |
| let token_uri = DEFAULT_GOOGLE_TOKEN_URI; | |
| let now = Utc::now().timestamp(); | |
| let claims = FcmJwtClaims { | |
| iss: &service_account.client_email, | |
| scope: GOOGLE_TOKEN_SCOPE, | |
| aud: token_uri, | |
| exp: now + 3600, | |
| iat: now - 30, | |
| }; | |
| let header = Header::new(Algorithm::RS256); | |
| let signing_key = EncodingKey::from_rsa_pem(service_account.private_key.as_bytes()) | |
| .map_err(|e| PushError::InvalidSecret(e.to_string()))?; | |
| let assertion = encode(&header, &claims, &signing_key)?; | |
| let response = transport | |
| .client | |
| .post(token_uri) | |
| .form(&[ | |
| ("grant_type", "urn:ietf:params:oauth:grant-type:jwt-bearer"), | |
| ("assertion", assertion.as_str()), | |
| ]) | |
| .send() | |
| .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/push/fcm.rs` around lines 174 - 199, The OAuth token exchange in fcm.rs
currently trusts service_account.token_uri, which allows a project-scoped secret
to redirect the signed JWT assertion to an arbitrary endpoint. Update the token
request flow in the FcmJwtClaims / send-token logic to ignore the
secret-provided token_uri and always use Google’s fixed OAuth token endpoint, or
enforce a strict allowlist that only permits the canonical Google URL before
calling transport.client.post.
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>
AnthonyRonning
force-pushed
the
codex/agent-aead-clean-rebase
branch
from
65149ea to
905c657
Compare
| start_push_worker(app_state.clone()); | ||
| start_schedule_worker(app_state.clone()); |
There was a problem hiding this comment.
🚩 Push worker and schedule worker start unconditionally regardless of app mode
Both start_push_worker and start_schedule_worker are started unconditionally at src/main.rs:3292-3293, but the agent routes at src/web/agent/mod.rs:514 are gated behind AppMode::Local | AppMode::Dev. This means the background workers will run in production even though the agent endpoints are disabled. The push worker processing notifications is likely intentional (push notifications may be enqueued by other code paths), but the schedule worker materializing and executing scheduled agent runs in production when no schedules can be created through the API seems unnecessary. It won't cause errors (there simply won't be any schedules to process), but it does consume resources polling the database every 5 seconds.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/nitro-deploy.md (1)
594-603: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the Markdown lint violations in this new section.
Line 605 skips from
##to####, and the fenced blocks here are missing language tags, so this section will keep trippingmarkdownlint. Use###for the provider subsections and label the fences (bash,yaml,ini/systemd).Example cleanup
-``` +```bash sudo vim /etc/nitro_enclaves/vsock-proxy.yaml-
+yaml
- {address: api.push.apple.com, port: 443}
- {address: api.sandbox.push.apple.com, port: 443}
- {address: fcm.googleapis.com, port: 443}
-#### APNs Production +### APNs Production -``` +```bash sudo vim /etc/systemd/system/vsock-apns-prod-proxy.service-
+ini
[Unit]
Description=Vsock APNs Production Proxy Service
...Also applies to: 605-605, 608-624, 627-643, 646-662, 666-684
🤖 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 `@docs/nitro-deploy.md` around lines 594 - 603, The new Nitro deploy section has Markdown lint issues: the heading hierarchy jumps from ## to ####, and several fenced blocks in the provider setup docs are missing language tags. Update the provider subsections in docs/nitro-deploy.md to use ### instead of ####, and add appropriate fence labels for each block in the affected examples (for example, bash for commands, yaml for proxy config, and ini or systemd for unit files). Keep the existing content but adjust the section structure and fence annotations consistently across the APNs and FCM sections.Source: Linters/SAST tools
🤖 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 `@entrypoint.sh`:
- Around line 328-331: Make the push provider host overrides in entrypoint.sh
idempotent instead of unconditionally appending to /etc/hosts. Update the block
that adds api.push.apple.com, api.sandbox.push.apple.com, and fcm.googleapis.com
so it first removes any existing mappings or replaces them in place before
writing the current loopback IPs, ensuring repeated starts do not create
duplicate/stale entries. Use the existing /etc/hosts update block as the target
for the fix.
---
Nitpick comments:
In `@docs/nitro-deploy.md`:
- Around line 594-603: The new Nitro deploy section has Markdown lint issues:
the heading hierarchy jumps from ## to ####, and several fenced blocks in the
provider setup docs are missing language tags. Update the provider subsections
in docs/nitro-deploy.md to use ### instead of ####, and add appropriate fence
labels for each block in the affected examples (for example, bash for commands,
yaml for proxy config, and ini or systemd for unit files). Keep the existing
content but adjust the section structure and fence annotations consistently
across the APNs and FCM sections.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 87d6f82b-7f00-47c9-87dd-9cc48bd896b9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (59)
Cargo.tomldocs/nitro-deploy.mdentrypoint.shflake.nixmigrations/2026-03-07-120000_push_notifications_v1/down.sqlmigrations/2026-03-07-120000_push_notifications_v1/up.sqlmigrations/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/os_flags.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
✅ Files skipped from review due to trivial changes (3)
- src/web/responses/storage.rs
- src/web/platform/mod.rs
- migrations/2026-03-07-120000_push_notifications_v1/down.sql
🚧 Files skipped from review as they are similar to previous changes (52)
- src/web/responses/handlers.rs
- src/web/mod.rs
- src/models/agent_background_grants.rs
- src/web/responses/conversions.rs
- migrations/2026-06-26-224528_agent_background_push_aead/down.sql
- src/web/agent/vision.rs
- src/models/project_settings.rs
- flake.nix
- src/security_invariants.rs
- src/models/conversation_summaries.rs
- src/models/mod.rs
- migrations/2026-06-26-224528_agent_background_push_aead/up.sql
- src/models/user_preferences.rs
- src/models/user_embeddings.rs
- src/push/crypto.rs
- migrations/2026-03-07-120000_push_notifications_v1/up.sql
- src/web/agent/compaction.rs
- Cargo.toml
- src/models/push_devices.rs
- src/web/rag.rs
- src/web/openai.rs
- src/models/notification_events.rs
- src/models/memory_blocks.rs
- src/web/platform/common.rs
- src/web/agent/signatures.rs
- src/web/agent/tools.rs
- src/push/fcm.rs
- src/models/agent_schedule_runs.rs
- src/web/responses/conversations.rs
- src/web/agent/reactions.rs
- src/web/login_routes.rs
- src/models/schema.rs
- src/seed_wrapping.rs
- src/push/apns.rs
- src/main.rs
- src/web/agent/runtime.rs
- src/agent_background.rs
- src/web/platform/project_routes.rs
- src/models/agent_schedules.rs
- src/web/push.rs
- src/db.rs
- src/models/notification_deliveries.rs
- src/aead_db_tamper_tests.rs
- src/push/binding.rs
- src/push/worker.rs
- src/push/mod.rs
- src/models/agents.rs
- src/web/agent/mod.rs
- src/web/agent/schedules.rs
- src/models/responses.rs
- src/brave.rs
- src/rag.rs
| # Add push provider hostnames to /etc/hosts | ||
| 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 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the /etc/hosts override idempotent.
These unconditional appends accumulate duplicate entries across restarts, and a stale earlier mapping can win because host resolution stops at the first matching line. Later updates to these loopback IPs may never take effect.
One way to upsert the mappings
-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
+for host in api.push.apple.com api.sandbox.push.apple.com fcm.googleapis.com; do
+ sed -i "/[[:space:]]${host//./\\.}\$/d" /etc/hosts
+done
+cat >> /etc/hosts <<'EOF'
+127.0.0.21 api.push.apple.com
+127.0.0.22 api.sandbox.push.apple.com
+127.0.0.20 fcm.googleapis.com
+EOF
log "Added APNs and FCM domains to /etc/hosts"📝 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.
| # Add push provider hostnames to /etc/hosts | |
| 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 | |
| # Add push provider hostnames to /etc/hosts | |
| for host in api.push.apple.com api.sandbox.push.apple.com fcm.googleapis.com; do | |
| sed -i "/[[:space:]]${host//./\\.}\$/d" /etc/hosts | |
| done | |
| cat >> /etc/hosts <<'EOF' | |
| 127.0.0.21 api.push.apple.com | |
| 127.0.0.22 api.sandbox.push.apple.com | |
| 127.0.0.20 fcm.googleapis.com | |
| EOF | |
| log "Added APNs and FCM domains to /etc/hosts" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[style] 329-329: Consider using { cmd1; cmd2; } >> file instead of individual redirects.
(SC2129)
🤖 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 - 331, Make the push provider host overrides
in entrypoint.sh idempotent instead of unconditionally appending to /etc/hosts.
Update the block that adds api.push.apple.com, api.sandbox.push.apple.com, and
fcm.googleapis.com so it first removes any existing mappings or replaces them in
place before writing the current loopback IPs, ensuring repeated starts do not
create duplicate/stale entries. Use the existing /etc/hosts update block as the
target for the fix.
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>
AnthonyRonning
force-pushed
the
codex/agent-aead-clean-rebase
branch
from
905c657 to
b0e4fe8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/web/agent/tools.rs (2)
264-306: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject an empty
oldvalue inmemory_replace.
value.replace("", new)inserts the replacement at every string boundary, producing surprising or oversized memory updates.Proposed fix
let Some(new) = args.get("new") else { return ToolResult::error("'new' argument required"); }; + if old.is_empty() { + return ToolResult::error("'old' must not be empty"); + } let mut conn = match self.state.db.get_pool().get() {🤖 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 264 - 306, The memory_replace flow in the tool handler accepts an empty "old" value and then reaches value.replace(old, new), which can create unexpected updates; add an explicit validation for old being non-empty before the contains/replace logic and return a ToolResult::error from the same function path when it is empty.
200-228: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid lost memory updates during concurrent tool runs.
Each memory tool reads/decrypts a block, computes a new value, then writes through a separate helper, so two overlapping agent runs can overwrite each other’s changes. Move the read-modify-write into one transaction with row locking or an optimistic version check.
Also applies to: 276-308, 361-391, 445-471
🤖 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 200 - 228, The memory block update flow in update_block_value can lose concurrent writes because the read/decrypt and write steps are split across separate helpers. Refactor the memory tool paths that call default_block_value, decrypt_with_key, and NewMemoryBlock::insert_or_update so the read-modify-write happens atomically in one transaction, using row locking or an optimistic version check to detect conflicting updates. Apply the same concurrency-safe pattern to the other affected tool handlers noted in the review comment so overlapping agent runs cannot overwrite each other’s changes.
🤖 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 `@docs/nitro-deploy.md`:
- Around line 594-680: The new Nitro deploy section has markdownlint issues:
several fenced code blocks in the added APNs/FCM instructions are missing
language tags, and the heading hierarchy jumps from a second-level heading to
fourth-level headings. Update the affected fences in this section to use the
appropriate tags (bash, yaml, ini) and change the provider subheadings to
third-level headings so the structure is consistent and lint-compliant.
In `@src/web/agent/tools.rs`:
- Around line 896-908: The `conn` checkout in `set_user_preference` is happening
before an awaited encryption step inside `runtime::upsert_user_preference`,
which can hold a pooled DB connection idle across async work. Refactor so
encryption and value preparation complete before acquiring the connection, or
move the `self.state.db.get_pool().get()` checkout to after the await, keeping
the DB connection held only during the actual write path.
- Around line 209-210: The memory size check in the tool that handles core
memory updates is using String::len(), which counts UTF-8 bytes instead of
characters. Update the validation in the relevant branch of
src/web/agent/tools.rs to count Unicode scalar characters for new_value before
comparing against MAX_CORE_MEMORY_BLOCK_CHARS, and keep the existing BadRequest
path when the character limit is exceeded.
---
Outside diff comments:
In `@src/web/agent/tools.rs`:
- Around line 264-306: The memory_replace flow in the tool handler accepts an
empty "old" value and then reaches value.replace(old, new), which can create
unexpected updates; add an explicit validation for old being non-empty before
the contains/replace logic and return a ToolResult::error from the same function
path when it is empty.
- Around line 200-228: The memory block update flow in update_block_value can
lose concurrent writes because the read/decrypt and write steps are split across
separate helpers. Refactor the memory tool paths that call default_block_value,
decrypt_with_key, and NewMemoryBlock::insert_or_update so the read-modify-write
happens atomically in one transaction, using row locking or an optimistic
version check to detect conflicting updates. Apply the same concurrency-safe
pattern to the other affected tool handlers noted in the review comment so
overlapping agent runs cannot overwrite each other’s changes.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bee93486-6002-4a39-a3f1-5c2c1b508630
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
Cargo.tomldocs/nitro-deploy.mdentrypoint.shmigrations/2026-03-07-120000_push_notifications_v1/down.sqlmigrations/2026-03-07-120000_push_notifications_v1/up.sqlmigrations/2026-06-26-224528_agent_background_push_aead/down.sqlmigrations/2026-06-26-224528_agent_background_push_aead/up.sqlsrc/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/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_preferences.rssrc/push/apns.rssrc/push/binding.rssrc/push/crypto.rssrc/push/fcm.rssrc/push/mod.rssrc/push/worker.rssrc/security_invariants.rssrc/seed_wrapping.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/login_routes.rssrc/web/mod.rssrc/web/platform/common.rssrc/web/platform/mod.rssrc/web/platform/project_routes.rssrc/web/push.rssrc/web/responses/conversions.rssrc/web/responses/handlers.rssrc/web/responses/storage.rs
💤 Files with no reviewable changes (1)
- src/jwt.rs
✅ Files skipped from review due to trivial changes (2)
- migrations/2026-03-07-120000_push_notifications_v1/down.sql
- src/models/mod.rs
🚧 Files skipped from review as they are similar to previous changes (39)
- src/web/responses/storage.rs
- Cargo.toml
- src/models/agent_background_grants.rs
- migrations/2026-06-26-224528_agent_background_push_aead/down.sql
- src/web/responses/handlers.rs
- migrations/2026-06-26-224528_agent_background_push_aead/up.sql
- src/push/apns.rs
- src/web/platform/common.rs
- src/web/platform/mod.rs
- src/security_invariants.rs
- src/push/crypto.rs
- src/web/responses/conversions.rs
- src/models/notification_events.rs
- src/models/user_preferences.rs
- src/web/platform/project_routes.rs
- src/push/mod.rs
- src/models/agent_schedules.rs
- src/web/login_routes.rs
- src/models/schema.rs
- src/models/project_settings.rs
- src/web/agent/reactions.rs
- src/seed_wrapping.rs
- src/push/worker.rs
- src/models/agent_schedule_runs.rs
- src/models/push_devices.rs
- src/web/push.rs
- migrations/2026-03-07-120000_push_notifications_v1/up.sql
- src/web/agent/runtime.rs
- src/models/notification_deliveries.rs
- src/web/mod.rs
- src/push/fcm.rs
- src/push/binding.rs
- src/web/agent/schedules.rs
- src/web/agent/signatures.rs
- src/web/agent/mod.rs
- src/models/agents.rs
- src/models/responses.rs
- src/main.rs
- src/brave.rs
| ``` | ||
| sudo vim /etc/nitro_enclaves/vsock-proxy.yaml | ||
| ``` | ||
|
|
||
| Add these lines: | ||
| ``` | ||
| - {address: api.push.apple.com, port: 443} | ||
| - {address: api.sandbox.push.apple.com, port: 443} | ||
| - {address: fcm.googleapis.com, port: 443} | ||
| ``` | ||
|
|
||
| #### APNs Production | ||
| Now create a service that spins this up automatically: | ||
|
|
||
| ``` | ||
| sudo vim /etc/systemd/system/vsock-apns-prod-proxy.service | ||
| ``` | ||
|
|
||
| ``` | ||
| [Unit] | ||
| Description=Vsock APNs Production Proxy Service | ||
| After=network.target | ||
|
|
||
| [Service] | ||
| User=root | ||
| ExecStart=/usr/bin/vsock-proxy 8024 api.push.apple.com 443 | ||
| Restart=always | ||
|
|
||
| [Install] | ||
| WantedBy=multi-user.target | ||
| ``` | ||
|
|
||
| #### APNs Sandbox | ||
| ``` | ||
| sudo vim /etc/systemd/system/vsock-apns-sandbox-proxy.service | ||
| ``` | ||
|
|
||
| ``` | ||
| [Unit] | ||
| Description=Vsock APNs Sandbox Proxy Service | ||
| After=network.target | ||
|
|
||
| [Service] | ||
| User=root | ||
| ExecStart=/usr/bin/vsock-proxy 8025 api.sandbox.push.apple.com 443 | ||
| Restart=always | ||
|
|
||
| [Install] | ||
| WantedBy=multi-user.target | ||
| ``` | ||
|
|
||
| #### FCM | ||
| ``` | ||
| sudo vim /etc/systemd/system/vsock-fcm-proxy.service | ||
| ``` | ||
|
|
||
| ``` | ||
| [Unit] | ||
| Description=Vsock FCM Proxy Service | ||
| After=network.target | ||
|
|
||
| [Service] | ||
| User=root | ||
| ExecStart=/usr/bin/vsock-proxy 8029 fcm.googleapis.com 443 | ||
| Restart=always | ||
|
|
||
| [Install] | ||
| WantedBy=multi-user.target | ||
| ``` | ||
|
|
||
| Activate services: | ||
|
|
||
| ``` | ||
| sudo systemctl daemon-reload | ||
| sudo systemctl enable vsock-apns-prod-proxy.service | ||
| sudo systemctl start vsock-apns-prod-proxy.service | ||
| sudo systemctl status vsock-apns-prod-proxy.service | ||
| sudo systemctl enable vsock-apns-sandbox-proxy.service | ||
| sudo systemctl start vsock-apns-sandbox-proxy.service | ||
| sudo systemctl status vsock-apns-sandbox-proxy.service | ||
| sudo systemctl enable vsock-fcm-proxy.service | ||
| sudo systemctl start vsock-fcm-proxy.service | ||
| sudo systemctl status vsock-fcm-proxy.service | ||
| ``` | ||
|
|
||
| A restart of these should not be needed but if you need to: | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the markdownlint violations in this new section.
The added fences are missing language tags, and Line 605 jumps from ## to ####, so this section will keep tripping MD040/MD001. Please tag the blocks (bash, yaml, ini) and make the provider headings ###.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 594-594: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 599-599: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 605-605: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
[warning] 608-608: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 612-612: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 627-627: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 631-631: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 646-646: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 650-650: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 666-666: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 680-680: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/nitro-deploy.md` around lines 594 - 680, The new Nitro deploy section
has markdownlint issues: several fenced code blocks in the added APNs/FCM
instructions are missing language tags, and the heading hierarchy jumps from a
second-level heading to fourth-level headings. Update the affected fences in
this section to use the appropriate tags (bash, yaml, ini) and change the
provider subheadings to third-level headings so the structure is consistent and
lint-compliant.
Source: Linters/SAST tools
| if new_value.len() > MAX_CORE_MEMORY_BLOCK_CHARS { | ||
| return Err(ApiError::BadRequest); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Count characters, not bytes, for the memory character limit.
String::len() counts UTF-8 bytes, so non-ASCII memory content can hit MAX_CORE_MEMORY_BLOCK_CHARS early.
Proposed fix
- if new_value.len() > MAX_CORE_MEMORY_BLOCK_CHARS {
+ if new_value.chars().count() > MAX_CORE_MEMORY_BLOCK_CHARS {
return Err(ApiError::BadRequest);
}📝 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.
| if new_value.len() > MAX_CORE_MEMORY_BLOCK_CHARS { | |
| return Err(ApiError::BadRequest); | |
| if new_value.chars().count() > MAX_CORE_MEMORY_BLOCK_CHARS { | |
| return Err(ApiError::BadRequest); |
🤖 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 209 - 210, The memory size check in the
tool that handles core memory updates is using String::len(), which counts UTF-8
bytes instead of characters. Update the validation in the relevant branch of
src/web/agent/tools.rs to count Unicode scalar characters for new_value before
comparing against MAX_CORE_MEMORY_BLOCK_CHARS, and keep the existing BadRequest
path when the character limit is exceeded.
| let mut conn = match self.state.db.get_pool().get() { | ||
| Ok(c) => c, | ||
| Err(_) => return ToolResult::error("Database connection error"), | ||
| }; | ||
|
|
||
| match runtime::upsert_user_preference( | ||
| &mut conn, | ||
| self.user_key.as_ref(), | ||
| self.user_id, | ||
| resolved_key, | ||
| Some(value.clone()), | ||
| ) | ||
| .await |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t hold a DB connection across async preference encryption.
conn is checked out before .await, while upsert_user_preference awaits encryption before doing DB work. Split encryption from the DB write or acquire the connection after the await to avoid pool starvation under load.
🤖 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 896 - 908, The `conn` checkout in
`set_user_preference` is happening before an awaited encryption step inside
`runtime::upsert_user_preference`, which can hold a pooled DB connection idle
across async work. Refactor so encryption and value preparation complete before
acquiring the connection, or move the `self.state.db.get_pool().get()` checkout
to after the await, keeping the DB connection held only during the actual write
path.
2 participants
Summary
masteras a clean seven-commit stack.Verification
nix develop -c cargo checkSummary by CodeRabbit