perf: bound the remaining full-agent plan and change-set reads#3571
Conversation
Three reads still loaded an agent's whole history and filtered in Dart. - _plannerOwnsDay called getCaptureEventMetaByAgentId with no day filter and scanned with metas.any(). It runs on every day-agent identity resolution — ahead of every day-view read — so it was the hottest remaining unbounded path. PR #3569 made captures day-indexed, so it is now a single indexed lookup limited to one row. - The plan writer loaded every dayPlan the agent ever wrote to serve a 7-day lookback; it now asks for exactly the window's days in one read. - The change-set editor loaded every changeSet ever proposed to find the pending ones; changeSet stores its status as the indexed subtype, so the confirmed/rejected history is never read. Adds getEntitiesByAgentIdAndSubtypes for the range case, so a lookback costs one round trip rather than one query per day.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
📝 WalkthroughWalkthroughAdds a multi-subtype agent-entity repository query with SQLite-safe batching, updates daily-agent plan and capture lookups to use subtype indexes, and aligns unit and integration test repositories with the new filtering behavior. ChangesSubtype-Scoped Agent Entity Reads
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DayAgentPlanWriter
participant AgentRepository
participant AgentRepoCore
participant SQLite
DayAgentPlanWriter->>AgentRepository: Request day plans for lookback subtypes
AgentRepository->>AgentRepoCore: Forward multi-subtype query
AgentRepoCore->>SQLite: Run chunked indexed IN queries
SQLite-->>AgentRepoCore: Return matching non-deleted rows
AgentRepoCore-->>AgentRepository: Return converted entities
AgentRepository-->>DayAgentPlanWriter: Return filtered day plans
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3571 +/- ##
=======================================
Coverage 99.22% 99.22%
=======================================
Files 1781 1781
Lines 130336 130359 +23
=======================================
+ Hits 129322 129345 +23
Misses 1014 1014
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/daily_os_next/agents/service/day_agent_plan_editor.dart (1)
63-88: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftBackfill
changeSetsubtype-from-status from the schema change.
pendingPlanDiffsForDaynow depends onagent_entities.subtype = 'pending'forchangeSetrows, andAgentDbConversions.toEntityCompanionderiveschangeSetsubtype fromstatus, but existing legacy rows are not covered by the schema v17 day-subtype backfill. Add a migration/backfill forchangeSet.subtype = json_extract(serialized, '$.status')or restore/remove the narrowed DB filter so pending diffs on old installs are not silently hidden.🤖 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 `@lib/features/daily_os_next/agents/service/day_agent_plan_editor.dart` around lines 63 - 88, Backfill legacy changeSet rows so their agent_entities.subtype is populated from json_extract(serialized, '$.status') during the schema v17 migration, covering existing installs before pendingPlanDiffsForDay queries them. Update the migration/backfill logic associated with the v17 day-subtype change, or remove the narrowed subtype filter in pendingPlanDiffsForDay until the backfill is guaranteed; preserve pending-only filtering for correctly migrated rows.
🧹 Nitpick comments (2)
test/features/daily_os_next/agents/service/day_agent_plan_service_test.dart (1)
4698-4715: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
stubEntitiesWithChangeSetsignoresagentId, unlike the neighboringbySubtypehelper.The stub matches only on
subtype, notagentId. Today'spendingPlanDiffsForDaytests all use a single, non-Daily-OS_agentId, so this doesn't currently produce a false pass, but it diverges from the real repository contract and frombySubtypeabove (which does matchagentId). If a future test in this group exercises the multi-owner path (coordinator + per-day agent), this stub would silently return the same entities for every owner, masking a real mismatch.♻️ Proposed fix: match agentId too
).thenAnswer((invocation) async { + final requestedAgentId = invocation.positionalArguments.single as String; final subtype = invocation.namedArguments[`#subtype`] as String; return [ for (final entity in entities) - if (AgentDbConversions.entitySubtype(entity) == subtype) entity, + if (entity.agentId == requestedAgentId && + AgentDbConversions.entitySubtype(entity) == subtype) + entity, ]; });🤖 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 `@test/features/daily_os_next/agents/service/day_agent_plan_service_test.dart` around lines 4698 - 4715, Update stubEntitiesWithChangeSets to capture the requested agentId from the getEntitiesByAgentIdAndSubtype invocation and filter entities by both matching agentId and AgentDbConversions.entitySubtype(entity) matching subtype, consistent with the neighboring bySubtype helper and repository contract.lib/features/agents/database/agent_repo_core.dart (1)
214-253: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid forcing
INDEXED BYin this query.This method hardcodes
INDEXED BY idx_agent_entities_agent_type_sub, while the single-subtype path above lets the drift query use the index naturally. SQLite rejectsINDEXED BYif the named index is missing or unusable, creating a future breakage risk if the index is ever renamed/migrated. Drop the hint here to keep this read resilient.🛡️ Proposed fix: drop the forced index hint
final rows = await _db .customSelect( 'SELECT * FROM agent_entities ' - 'INDEXED BY idx_agent_entities_agent_type_sub ' 'WHERE agent_id = ? AND type = ? ' 'AND subtype IN ($placeholders) ' 'AND deleted_at IS NULL ' 'ORDER BY created_at DESC, id DESC',🤖 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 `@lib/features/agents/database/agent_repo_core.dart` around lines 214 - 253, Remove the forced INDEXED BY idx_agent_entities_agent_type_sub clause from the SQL in getEntitiesByAgentIdAndSubtypes, allowing SQLite to select an available index naturally while leaving the query filters, ordering, and result conversion unchanged.
🤖 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 `@test/features/daily_os_next/agents/service/day_agent_service_test.dart`:
- Around line 1133-1138: Update the repository verification in the
DayAgentService ownership probe test around getEntitiesByAgentIdAndSubtype to
assert the concrete limit value of 1 instead of allowing any named limit. Keep
the existing agent ID, capture type, and testDayId subtype matchers unchanged.
---
Outside diff comments:
In `@lib/features/daily_os_next/agents/service/day_agent_plan_editor.dart`:
- Around line 63-88: Backfill legacy changeSet rows so their
agent_entities.subtype is populated from json_extract(serialized, '$.status')
during the schema v17 migration, covering existing installs before
pendingPlanDiffsForDay queries them. Update the migration/backfill logic
associated with the v17 day-subtype change, or remove the narrowed subtype
filter in pendingPlanDiffsForDay until the backfill is guaranteed; preserve
pending-only filtering for correctly migrated rows.
---
Nitpick comments:
In `@lib/features/agents/database/agent_repo_core.dart`:
- Around line 214-253: Remove the forced INDEXED BY
idx_agent_entities_agent_type_sub clause from the SQL in
getEntitiesByAgentIdAndSubtypes, allowing SQLite to select an available index
naturally while leaving the query filters, ordering, and result conversion
unchanged.
In `@test/features/daily_os_next/agents/service/day_agent_plan_service_test.dart`:
- Around line 4698-4715: Update stubEntitiesWithChangeSets to capture the
requested agentId from the getEntitiesByAgentIdAndSubtype invocation and filter
entities by both matching agentId and AgentDbConversions.entitySubtype(entity)
matching subtype, consistent with the neighboring bySubtype helper and
repository contract.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 04cf1ead-b90c-41fa-aadf-8d7046884746
📒 Files selected for processing (10)
lib/features/agents/database/agent_repo_core.dartlib/features/agents/database/agent_repository.dartlib/features/daily_os_next/agents/service/day_agent_plan_editor.dartlib/features/daily_os_next/agents/service/day_agent_plan_writer.dartlib/features/daily_os_next/agents/service/day_agent_service.darttest/features/agents/database/agent_repo_core_test.darttest/features/agents/database/agent_repository_test.darttest/features/daily_os_next/agents/service/day_agent_plan_service_test.darttest/features/daily_os_next/agents/service/day_agent_service_test.darttest/features/daily_os_next/integration/day_agent_pipeline_harness.dart
| () => repository.getEntitiesByAgentIdAndSubtype( | ||
| dailyOsPlannerAgentId, | ||
| type: AgentEntityTypes.capture, | ||
| subtype: testDayId, | ||
| limit: any(named: 'limit'), | ||
| ), |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Assert the bounded ownership-query limit.
limit: any(...) allows the test to pass if the implementation regresses to the repository default of -1 and performs an unbounded read. Assert the concrete bound used by DayAgentService (expected to be 1) instead.
Suggested change
- limit: any(named: 'limit'),
+ limit: 1,The PR objective requires this ownership probe to remain bounded.
📝 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.
| () => repository.getEntitiesByAgentIdAndSubtype( | |
| dailyOsPlannerAgentId, | |
| type: AgentEntityTypes.capture, | |
| subtype: testDayId, | |
| limit: any(named: 'limit'), | |
| ), | |
| () => repository.getEntitiesByAgentIdAndSubtype( | |
| dailyOsPlannerAgentId, | |
| type: AgentEntityTypes.capture, | |
| subtype: testDayId, | |
| limit: 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 `@test/features/daily_os_next/agents/service/day_agent_service_test.dart`
around lines 1133 - 1138, Update the repository verification in the
DayAgentService ownership probe test around getEntitiesByAgentIdAndSubtype to
assert the concrete limit value of 1 instead of allowing any named limit. Keep
the existing agent ID, capture type, and testDayId subtype matchers unchanged.
1 participant
Fourth in the "day planner must not degrade as history accumulates" series,
after #3564, #3566 and #3569. Closes out the load-everything-then-filter-in-Dart
pattern.
What was still unbounded
_plannerOwnsDay(day_agent_service.dart) calledgetCaptureEventMetaByAgentIdwith no day filter and scanned the result withmetas.any(...). It runs on every day-agent identity resolution — that is,ahead of every day-view read — which made it the hottest remaining unbounded
path, hotter than the two providers #3569 fixed. Its docstring claimed the
narrow projection "keeps per-wake cost flat instead of O(all captures)", which
is true per row and false overall: the row count and the
json_extractper rowstill scaled with the coordinator's whole capture history.
Because #3569 made captures day-indexed, this is now a single indexed lookup
capped at one row.
The plan writer loaded every
dayPlanthe agent had ever written to servea 7-day lookback.
dayPlanalready stored its day as the subtype, so it nowasks for exactly the window's days.
The change-set editor loaded every
changeSetever proposed to find thepending ones.
changeSetstores its status as the subtype, so theconfirmed/rejected history — which grows with every diff the user has ever
acted on — is no longer read at all.
New repository method
getEntitiesByAgentIdAndSubtypes(plural) for the range case, so a lookback isone round trip instead of one query per day. Chunked through
sqliteInClauseChunkslike the other batched reads.Testing
1902 daily_os_next + 496 agents/database tests pass; analyzer and formatter
clean; every changed line covered.
Worth noting about the test changes: several stubs previously returned entities
regardless of what was asked for, so they would have kept passing even if the
production filter were wrong. They now resolve subtypes through
AgentDbConversions.entitySubtype— the same projection the writer uses — so afake can no longer disagree with the real table. The in-memory harness
repository got the same treatment.
No CHANGELOG entry — no user-visible behavior change.
Summary by CodeRabbit
New Features
Bug Fixes
Tests