lib/features/ai/model/ai_call_impact.dart (1)

15-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider Freezed for consistency.

MeliousCallImpact hand-rolls ==/hashCode/toString while the sibling AiConsumptionEvent model in this PR uses @freezed. Not required, but converting would reduce boilerplate and match the pattern used elsewhere in this cohort.

🤖 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/ai/model/ai_call_impact.dart` around lines 15 - 130,
MeliousCallImpact is hand-implementing value semantics while the sibling
AiConsumptionEvent uses Freezed, so align this model with that pattern for
consistency. Update MeliousCallImpact to use `@freezed` and move the current
constructor/factory parsing logic into the generated-style structure, keeping
fromResponseJson and the existing fields intact. Remove the manual ==, hashCode,
and toString overrides once Freezed covers them, and make sure the class remains
easy to locate by its MeliousCallImpact symbol and fromResponseJson factory.

lib/features/ai/repository/melious_inference_repository.dart (3)

1385-1533: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding a timeout-failure test for the non-streaming impact path.

Other network paths in this file (listModels, generateImage) have explicit timeout tests, but the new _postChatCompletion/_nonStreamingChat path (used whenever an impactCollector is supplied) doesn't. Given the increased _chatCompletionTimeout and its role in cost/impact tracking correctness, a Duration.zero timeout test mirroring the existing pattern would close the gap.

🤖 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/ai/repository/melious_inference_repository.dart` around lines
1385 - 1533, Add a timeout-failure test for the non-streaming impact path in
melious_inference_repository.dart by covering the _postChatCompletion /
_nonStreamingChat flow used when an impactCollector is provided. Mirror the
existing timeout test patterns used for listModels and generateImage, and verify
that a Duration.zero timeout causes the expected failure through the
non-streaming chat completion path while respecting the _chatCompletionTimeout
behavior. Focus the test around the _postChatCompletion and _nonStreamingChat
symbols so the coverage stays aligned with the impact-tracking branch.

---

550-555: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

_asInt duplicates the pre-existing _integerValue helper.

Both are private static methods with identical bodies (int/num/String → nullable int coercion). Consolidate into one.

♻️ Proposed consolidation

-  static CompletionUsage? _parseUsage(Object? raw) {
+  static CompletionUsage? _parseUsage(Object? raw) {
     if (raw is! Map) return null;
     final map = raw.cast<String, dynamic>();
-    final prompt = _asInt(map['prompt_tokens']);
-    final completion = _asInt(map['completion_tokens']);
-    final cached = _asInt(map['cached_tokens']);
+    final prompt = _integerValue(map['prompt_tokens']);
+    final completion = _integerValue(map['completion_tokens']);
+    final cached = _integerValue(map['cached_tokens']);
     return CompletionUsage(
       promptTokens: prompt,
       completionTokens: completion,
       totalTokens:
-          _asInt(map['total_tokens']) ?? (prompt ?? 0) + (completion ?? 0),
+          _integerValue(map['total_tokens']) ?? (prompt ?? 0) + (completion ?? 0),
       promptTokensDetails: cached != null
           ? PromptTokensDetails(cachedTokens: cached)
           : null,
     );
   }

-  static int? _asInt(Object? value) {
-    if (value is int) return value;
-    if (value is num) return value.toInt();
-    if (value is String) return int.tryParse(value);
-    return null;
-  }

Also applies to: 948-953

🤖 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/ai/repository/melious_inference_repository.dart` around lines
550 - 555, The private static helper _asInt duplicates the existing
_integerValue conversion logic in melious_inference_repository.dart. Remove the
duplicate by reusing the shared helper from the repository class, and update the
call sites in the affected inference parsing paths to use the single coercion
method so the int/num/String-to-nullable-int behavior lives in one place only.

---

43-43: 🩺 Stability & Availability | 🔵 Trivial

Non-streaming impact path suppresses progress updates for the full call duration (up to 300s).

_nonStreamingChat yields nothing until the whole response (or the 300s timeout) resolves, so onProgress in UnifiedAiInferenceRepository won't fire incrementally for measured Melious calls — a UX regression relative to the streaming path for potentially long-running responses (e.g. reasoning models). This is an accepted trade-off for capturing environment_impact/billing_cost (only available on non-streaming responses), but worth surfacing since it affects perceived responsiveness.

Also applies to: 374-433

🤖 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/ai/repository/melious_inference_repository.dart` at line 43,
`_nonStreamingChat` in `MeliousInferenceRepository` blocks progress callbacks
for the full request, so make that behavior explicit where the non-streaming
path is chosen. Update the implementation/comments around
`_chatCompletionTimeout` and the
`_nonStreamingChat`/`UnifiedAiInferenceRepository` flow to clearly document that
`onProgress` will not fire incrementally for impact-aware Melious calls because
`environment_impact` and `billing_cost` require the non-streaming response. If
needed, surface this trade-off in the call path that selects streaming vs
non-streaming so future readers understand the responsiveness impact.

test/features/ai/repository/unified_ai_inference_repository_test.dart (1)

1286-1300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move the new fallback registration out of the test body, matching this file's own convention.

Every other registerFallbackValue(...) call in this suite lives in setUpAll() (lines 106-124); this one is registered inline inside a single test. Consider moving registerFallbackValue(consumption.makeConsumptionEvent()) to setUpAll() (or centralizing it in test/helpers/fallbacks.dart per convention) for consistency.

As per path instructions: "Centralize fallback values in test/helpers/fallbacks.dart and register with registerFallbackValue(...)."

🤖 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/ai/repository/unified_ai_inference_repository_test.dart` around
lines 1286 - 1300, The fallback registration for the consumption event is
inconsistent with this suite’s setup and should be centralized instead of done
inside the individual test. Move the
`registerFallbackValue(consumption.makeConsumptionEvent())` call out of the test
body and into the shared setup used by this file, preferably `setUpAll()` in
`unified_ai_inference_repository_test.dart` or the common fallback helper in
`test/helpers/fallbacks.dart`, keeping the `MockAiConsumptionRecorder` test
logic unchanged.

Source: Path instructions

test/features/ai/repository/inference_repository_interface_test.dart (1)

4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Minor inconsistency: impactCollector isn't captured like the other parameters.

Every other parameter here is recorded into a lastX field (e.g. lastSignatureCollector), but impactCollector is accepted and dropped. Harmless today since no test asserts on it, but consider adding lastImpactCollector for parity if a future test needs to verify pass-through.

Also applies to: 47-72

🤖 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/ai/repository/inference_repository_interface_test.dart` at line
4, The InferenceRepositoryInterface test setup is inconsistent because
impactCollector is passed in but not stored like the other captured parameters.
Update the relevant test helper/mocks in InferenceRepositoryInterface so it also
records lastImpactCollector alongside the existing lastSignatureCollector-style
fields, ensuring the collector passed through the interface can be asserted
later if needed.

lib/features/ai/services/skill_inference_runner.dart (2)

215-238: 📐 Maintainability & Code Quality | 🔵 Trivial

Duplicated stream-collection + usage-capture logic across three methods.

The CompletionUsage? usage; await for (...) { if (chunk.usage != null) usage = chunk.usage; ... } block is copy-pasted identically in runTranscription, runImageAnalysis, and runPromptGeneration. Consider extracting a small helper (e.g. Future<(String, CompletionUsage?)> _collectStream(Stream<...> stream)) that returns the concatenated content and captured usage, to avoid triple maintenance if the accumulation logic ever needs to change (e.g. handling multiple usage chunks).

Also applies to: 384-407, 538-561

🤖 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/ai/services/skill_inference_runner.dart` around lines 215 - 238,
The stream-consumption and usage-tracking logic is duplicated in
runTranscription, runImageAnalysis, and runPromptGeneration, making the same
accumulation block hard to maintain. Extract the repeated await for (...) loop
into a small helper on skill_inference_runner.dart, such as a private
_collectStream method that takes the response stream and returns both the
concatenated content and the last seen CompletionUsage. Update each of the three
methods to call that helper and keep the existing _recordConsumption call using
the returned usage and buffer content.

---

835-835: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Consider uuid.v4() instead of uuid.v1() for the consumption event id.

uuid.v1() embeds a timestamp and a node identifier that (per the uuid package's docs) is randomly generated once and then reused for subsequent v1 calls from the same instance — this can make consumption events linkable to the same originating session/device across synced records, and duplicates the timestamp already captured explicitly via createdAt: start. v4() (purely random) is the more conventional choice for an opaque record id and avoids any node-based correlation, especially since these events sync across devices via Matrix.

Please confirm how uuid (the shared instance referenced here) is configured elsewhere in the codebase — if a custom node option is ever supplied, verify it isn't derived from real hardware identifiers.

🤖 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/ai/services/skill_inference_runner.dart` at line 835, The
consumption event id in skill inference should use an opaque random UUID rather
than a time/node-based one. Update the id assignment in the consumption event
creation path that currently uses the shared `uuid` instance’s `v1()` call to
`uuid.v4()`, and keep `createdAt: start` as the time source. Also verify any
shared `uuid` configuration elsewhere does not supply a custom `node` value
derived from real hardware identifiers.

lib/features/ai_consumption/database/consumption_database.dart (1)

52-74: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

OFFSET-based pagination degrades on large tables.

Each batch re-scans and discards all prior rows, so total cost across all pages grows roughly O(n²) as the table (and offset) grows. For a high-volume, append-only table intended to log every AI backend call, this stream could see meaningful degradation on larger installs or full backfills.

Consider keyset pagination instead (track the last-seen id and query WHERE id > ? ORDER BY id LIMIT ?), which scales linearly regardless of table size.

♻️ Suggested keyset-pagination approach

-    var offset = 0;
+    String? lastId;
     while (true) {
       final rows = await customSelect(
-        'SELECT id, serialized FROM consumption_events '
-        'ORDER BY id LIMIT ? OFFSET ?',
-        variables: [Variable(batchSize), Variable(offset)],
+        'SELECT id, serialized FROM consumption_events '
+        '${lastId != null ? 'WHERE id > ? ' : ''}'
+        'ORDER BY id LIMIT ?',
+        variables: [
+          if (lastId != null) Variable(lastId),
+          Variable(batchSize),
+        ],
       ).get();

       if (rows.isEmpty) break;

       yield rows
           .map(
             (row) => (
               id: row.read<String>('id'),
               vectorClock: _extractVectorClock(row.read<String>('serialized')),
             ),
           )
           .toList();
-      offset += batchSize;
+      lastId = rows.last.read<String>('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 `@lib/features/ai_consumption/database/consumption_database.dart` around lines
52 - 74, The streamConsumptionEventsWithVectorClock method currently uses OFFSET
pagination, which will get slower as consumption_events grows. Update this
method to use keyset pagination instead by tracking the last seen id and
querying with a WHERE id > ? clause plus ORDER BY id LIMIT ?; keep the batching
behavior and loop termination the same, but remove the offset-based scan so the
pagination scales linearly.

lib/features/ai_consumption/database/consumption_database.drift (1)

76-81: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Drop the unused vector-clock query or route callers through it.

getConsumptionEventVectorClockById isn’t referenced in source, while ConsumptionRepository.getVectorClock still uses the same json_extract(... '$.vectorClock') SQL inline. ConsumptionDatabase._extractVectorClock is a separate path for the serialized batch stream, so the only duplicated lookup here is between the inline query and the named query.

🤖 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/ai_consumption/database/consumption_database.drift` around lines
76 - 81, The vector-clock lookup is duplicated:
`getConsumptionEventVectorClockById` in the Drift database schema is currently
unused, while `ConsumptionRepository.getVectorClock` still embeds the same
`json_extract(... '$.vectorClock')` query inline. Either remove the unused named
query from `consumption_database.drift` or update
`ConsumptionRepository.getVectorClock` to call the generated query method so
there is a single source of truth; leave
`ConsumptionDatabase._extractVectorClock` unchanged since it serves the
batch-stream path.

lib/features/ai/conversation/conversation_repository.dart (1)

280-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider clock.now() instead of raw DateTime.now() for turn timing.

turnStart (line 283) and the durationMs computation in _recordTurnConsumption (line 495) use raw DateTime.now(), while sibling workflow files in this same PR (e.g. event_agent_workflow.dart's final now = clock.now();) use package:clock for testability. Since durationMs/createdAt are now persisted, billed/audited consumption-tracking fields, using the injectable clock would keep this path consistent with the rest of the codebase and deterministically testable.

♻️ Proposed fix

-        final turnStart = DateTime.now();
+        final turnStart = clock.now();

-        durationMs: DateTime.now().difference(start).inMilliseconds,
+        durationMs: clock.now().difference(start).inMilliseconds,

Also applies to: 459-511

🤖 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/ai/conversation/conversation_repository.dart` around lines 280 -
284, The turn timing in ConversationRepository is using raw DateTime.now() in
both the turn start capture and the duration calculation path, which should be
aligned with the injectable clock used elsewhere. Update the turn-start logic in
the conversation flow and the timing math in _recordTurnConsumption to use
clock.now() instead of DateTime.now(), keeping the timing source consistent and
testable through the same clock-based pattern used in sibling workflow code.

test/features/sync/matrix/sync_event_processor_consumption_handlers_test.dart (1)

13-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Centralize the consumption-event fallback value instead of registering it inline.

registerFallbackValue(makeConsumptionEvent()) is registered directly in this test's setUpAll, alongside the shared registerSyncProcessorFallbacks() helper. As per path instructions, fallback values should be centralized in test/helpers/fallbacks.dart rather than declared ad hoc per test file.

♻️ Suggested consolidation

-  setUpAll(() {
-    registerSyncProcessorFallbacks();
-    registerFallbackValue(makeConsumptionEvent());
-  });
+  setUpAll(registerSyncProcessorFallbacks);

And add the AiConsumptionEvent fallback registration inside registerSyncProcessorFallbacks() in test/helpers/fallbacks.dart (or the shared helper it delegates to).

🤖 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/sync/matrix/sync_event_processor_consumption_handlers_test.dart`
around lines 13 - 16, The consumption-event fallback is being registered ad hoc
in the test file’s setUpAll, which should be centralized instead. Remove the
inline registerFallbackValue(makeConsumptionEvent()) call from
sync_event_processor_consumption_handlers_test and add the AiConsumptionEvent
fallback registration inside registerSyncProcessorFallbacks() in
test/helpers/fallbacks.dart (or the shared helper it calls), so this test only
relies on the shared fallback setup.

Source: Path instructions

lib/features/agents/workflow/template_evolution_session.dart (1)

254-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Cache the isRegistered check once.

getIt.isRegistered<AiConsumptionRecorder>() is evaluated twice for the same two fields. day_agent_workflow.dart in this same PR caches this check into a local (recordConsumption) and reuses it — worth mirroring here (and in template_evolution_workflow.dart, which has the same duplication) for consistency.

♻️ Proposed fix

+      final recordConsumption = getIt.isRegistered<AiConsumptionRecorder>();
       await conversationRepository.sendMessage(
         ...
         // Evolution attributes consumption to the template being improved.
-        consumptionAgentId: getIt.isRegistered<AiConsumptionRecorder>()
-            ? templateId
-            : null,
-        consumptionThreadId: getIt.isRegistered<AiConsumptionRecorder>()
-            ? conversationId
-            : null,
+        consumptionAgentId: recordConsumption ? templateId : null,
+        consumptionThreadId: recordConsumption ? conversationId : null,

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/features/agents/workflow/template_evolution_session.dart` around lines
254 - 260, Cache the result of getIt.isRegistered<AiConsumptionRecorder>() once
and reuse it for both consumptionAgentId and consumptionThreadId, instead of
evaluating it twice. Update the logic in the template evolution session flow by
introducing a local boolean like recordConsumption, mirroring the pattern used
in day_agent_workflow.dart, and apply the same cleanup in
template_evolution_workflow.dart where the duplication also exists.

test/features/agents/workflow/task_agent_workflow_test_helpers.dart (1)

34-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Forward the consumption IDs into sendMessageDelegate. MockConversationRepository.sendMessage accepts the new consumption fields, but the delegate type and call still drop them, so tests using this helper can’t assert the ownership IDs that TaskAgentWorkflow passes through.

🤖 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/agents/workflow/task_agent_workflow_test_helpers.dart` around
lines 34 - 46, Forward the new consumption ID fields through
MockConversationRepository.sendMessage by updating the sendMessageDelegate
signature in task_agent_workflow_test_helpers.dart to accept the
ownership/consumption IDs and passing them along from the sendMessage call site.
Make sure the delegate type stays aligned with
MockConversationRepository.sendMessage and TaskAgentWorkflow so tests can assert
the IDs are preserved end-to-end.

test/features/ai/conversation/conversation_repository_test.dart (1)

30-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract _registerConsumptionRecorder() to a shared test helper.

This helper is duplicated verbatim in test/features/ai/repository/unified_ai_inference_repository_test.dart. Since test/features/ai_consumption/test_utils.dart is already shared across several files in this PR (e.g., for makeConsumptionEvent()), moving this registration/teardown helper there too would avoid drift between the two copies.

♻️ Suggested consolidation

-MockAiConsumptionRecorder _registerConsumptionRecorder() {
-  final recorder = MockAiConsumptionRecorder();
-  when(() => recorder.record(any())).thenAnswer((_) async {});
-  if (getIt.isRegistered<AiConsumptionRecorder>()) {
-    getIt.unregister<AiConsumptionRecorder>();
-  }
-  getIt.registerSingleton<AiConsumptionRecorder>(recorder);
-  addTearDown(() {
-    if (getIt.isRegistered<AiConsumptionRecorder>()) {
-      getIt.unregister<AiConsumptionRecorder>();
-    }
-  });
-  return recorder;
-}
+// Moved to test/features/ai_consumption/test_utils.dart as
+// `registerConsumptionRecorder()`, imported here 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 `@test/features/ai/conversation/conversation_repository_test.dart` around lines
30 - 46, The `_registerConsumptionRecorder()` helper is duplicated across test
suites, so move the registration/teardown logic into the shared test utilities
used by `makeConsumptionEvent()` in `test_utils.dart` and update both
`conversation_repository_test.dart` and
`unified_ai_inference_repository_test.dart` to call that shared helper. Keep the
behavior the same for `MockAiConsumptionRecorder`, `getIt`, and `addTearDown`,
but centralize the implementation so the stub registration stays consistent and
does not drift between tests.

test/features/agents/workflow/template_evolution_workflow_test.dart (1)

290-331: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the AiConsumptionRecorder registration/teardown boilerplate into a shared test helper.

The register-singleton + isRegistered-guarded addTearDown unregister block is now duplicated verbatim across at least four workflow test files (soul_evolution, task_agent, project_agent, template_evolution). A shared helper (e.g. withMockAiConsumptionRecorder() in a common workflow test-utils file) would remove this repetition.

Based on learnings, in workflow test files under test/features/agents/workflow/, local stateful test doubles are fine, but shared boilerplate should move to a helper once a third occurrence needs the same pattern — that threshold has now been passed.

🤖 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/agents/workflow/template_evolution_workflow_test.dart` around
lines 290 - 331, The AiConsumptionRecorder setup/teardown block in this test is
duplicated across multiple workflow test files, so extract it into a shared
helper instead of keeping the registerSingleton plus guarded addTearDown
unregister logic inline. Add a common test utility (for example, a helper around
getIt.registerSingleton<AiConsumptionRecorder> and the matching teardown) and
update this test to use that helper, keeping the local workflow-specific stubs
like _TestConversationRepository and buildSessionWorkflow unchanged.

Source: Learnings

test/features/agents/workflow/event_agent_workflow_test.dart (1)

950-957: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consumption-recorder registration/teardown boilerplate duplicated.

This exact getIt.registerSingleton<AiConsumptionRecorder>(MockAiConsumptionRecorder()); addTearDown(...) block is identical to the one at lines 414-421. Consider a small registerConsumptionRecorder() test helper to avoid the copy-paste as more consumption-attribution tests are added.

🤖 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/agents/workflow/event_agent_workflow_test.dart` around lines
950 - 957, The AiConsumptionRecorder test setup is duplicated, so extract the
repeated
getIt.registerSingleton<AiConsumptionRecorder>(MockAiConsumptionRecorder()) plus
addTearDown unregistration into a small reusable helper such as
registerConsumptionRecorder(). Update the affected event_agent_workflow_test
cases to call that helper instead of inlining the boilerplate, and keep the
helper responsible for registering and unregistering the mock through getIt.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes