[thomasluizon]

What

Extends the single daily-summary generation to emit a second short field, insight, alongside summary — one gated, cached AI call now returns both.

• ISummaryService.GenerateSummaryAsync returns a DailySummaryContent(Summary, Insight) record.
• AiSummaryService requests { summary, insight } via the generalized AiCompletionClient.CompleteJsonAsync<T> (now takes systemPrompt + temperature + maxOutputTokens, mirroring CompleteTextAsync), keeping the warm Astra persona + temperature 0.7. Summary keeps its existing post-processing; insight is trimmed to <=70 chars; empty insight degrades gracefully.
• DailySummaryResponse gains an additive Insight field, cached in and read from the same IMemoryCache entry. No new endpoint, query key, AI call, or meter.

Why

Powers the Today AI insight chip (#221). Gating is by inheritance: the insight rides the same CanUseDailySummary + AiSummaryEnabled gates and the same once-per-time-bucket cache, so the cost/caching AC is satisfied structurally.

Backward compatibility

Insight is additive; old mobile clients ignore it. No EF/schema change -> no migration. Astra chat tool returns the response object (insight rides along additively); MCP get_daily_summary stays summary-only.

Tests

• GetDailySummaryQueryHandlerTests: insight propagated on fresh + cached paths.
• AiSummaryServiceTests: insight parsed, trimmed to <=70, empty-insight graceful, blank summary fails (stubbed chat transport).
• AiCompletionClientTests, GetDailySummaryToolTests, MCP HabitToolsTests updated for the new shapes.
• dotnet build + full dotnet test green (3729 tests).

Refs thomasluizon/orbit-ui-mobile#221

🤖 Generated with Claude Code

[claude]

Code Review: PR #256

Scope: PR #256 — feat(api): daily summary AI insight field (#221)
Recommendation: APPROVE

Summary

This PR extends the daily summary AI call to return a second field, insight, alongside summary, packaging both in a new DailySummaryContent record. The change is structurally clean: one generalized AI call, proper cache type migration, additive DTO field, and thorough test coverage for all new paths. No Critical or High findings survive review.

Findings

Critical

None.

High

None.

Medium

[MEDIUM] GetDailySummaryQueryValidator does not validate the Language parameter
· dimension: Backend hard rules (#13) — validation
· location: src/Orbit.Application/Habits/Validators/GetDailySummaryQueryValidator.cs
· issue: The validator checks DateFrom/DateTo range but has no rule on Language. An arbitrary string of any length flows through language resolution, into the cache key (summary:{userId}:{date}:{language}:{bucket}), and into LocaleHelper.GetAiLanguageName, which is then embedded verbatim into the AI prompt. The controller passes Language straight from the query string with no length bound.
· risk: A crafted long or adversarial language tag can bloat cache entries and attempt to manipulate the AI prompt.
· fix: Add one rule to the existing validator:

RuleFor(q => q.Language)
    .MaximumLength(20)
    .When(q => !string.IsNullOrEmpty(q.Language));

BCP-47 tags are at most ~35 chars; 20 covers every valid code and blocks injection attempts.
· reference: Orbit.Application/CLAUDE.md — "Validate ALL invalid/edge cases"; orbit-api hard rule: backend is source of truth.

Low / Info

[INFO] MCP get_daily_summary tool intentionally omits Insight — noted, no action
· location: src/Orbit.Api/Mcp/Tools/HabitTools.cs:517-518
· The MCP tool returns only s.Summary. PR body explicitly documents this as intentional ("MCP get_daily_summary stays summary-only"). If Astra should ever surface the insight chip in chat, this is the extension point.

[INFO] Insight is additive — backward-compat guard: PASS
· DailySummaryResponse gains a new string Insight field; Summary and FromCache are positionally unchanged. Old mobile clients ignore unknown JSON fields. No field was removed or renamed. Forward-compatible.

[INFO] CapToSentence second-loop bound is safe
· The security subagent flagged for (var i = maxChars; i >= 0; i--) as a potential index-out-of-range. Analysis: the loop is only reachable when text.Length > maxChars (the guard at line 344 short-circuits otherwise), so text[maxChars] is always a valid index. No bug.

Subagents

Agent	Verdict
security-reviewer	PASS — no critical/high; 1 medium elevated to findings above
contract-aligner	NOT VERIFIABLE — orbit-ui-mobile not checked out per scope

Validation

Check	Result
Build (dotnet)	N/A — skipped per scope
Tests (dotnet)	N/A — skipped per scope (PR body: 3 729 tests green)

What's good

• Single AI call for two fields: no second AI call, no second cache entry — both fields ride the existing CanUseDailySummary gate and the same time-bucket cache key. Zero cost regression.
• Type-safe cache migration: changing the cache value from string to DailySummaryContent is safe across a redeploy — old string entries simply miss (TryGetValue with the new generic type returns false), triggering a fresh AI call as the correct safe-degradation path.
• Graceful empty insight: content.Insight ?? string.Empty + CapToSentence means an AI that omits the field or returns empty produces an empty Insight, not a null propagation or failure.
• DailySummaryJson is private: the internal deserialization record never leaks past the service boundary — only DailySummaryContent (domain) and DailySummaryResponse (application) are public.
• CompleteJsonAsync refactor: the systemPrompt + userPrompt split mirrors CompleteTextAsync and removes the implicit generic system prompt, allowing the Astra persona to be applied consistently across all JSON calls.
• Test coverage is thorough: four new AiSummaryService tests use a real stubbed HTTP transport (not just mocking the interface), giving genuine end-to-end confidence in JSON deserialization, trimming, and empty-insight paths. All handler tests updated for the new DailySummaryContent shape.
• No EF migration: Insight is runtime AI-generated, not persisted.

Recommendation

One concretely-actionable Medium finding: add MaximumLength(20) on Language in GetDailySummaryQueryValidator — a one-liner. This is the only gap between this PR and a clean merge. All backward-compat and contract concerns are Info-level. Approve contingent on that validator rule.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
87.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

Recommendation: APPROVE -- 0 Critical, 0 High, 2 Medium, 1 Info

This PR adds an additive insight field to the daily summary response by migrating AiSummaryService to CompleteJsonAsync, returning a DailySummaryContent(Summary, Insight) record, and updating the query handler, cache, and all tests. One AI call, one cache entry, no new endpoint. No Critical or High findings.

MEDIUM: GET /api/habits/summary has no [DistributedRateLimit] (HabitsController.cs:78). The endpoint calls the AI on every cache miss with no per-user rate limit. GetRetrospective (line 104) has the same gap. CLAUDE.md requires [DistributedRateLimit] on abuse-prone endpoints; ChatController, AiController, UploadsController, and SupportController all comply. This PR raises maxOutputTokens 180 to 240, increasing per-call cost. Fix: add [DistributedRateLimit("ai-summary")] to both actions. Pre-existing gap, prioritize as a follow-up given the token-cost increase.

MEDIUM: StripMarkdownFences not applied to insight (AiSummaryService.cs:52-53). summary goes through StripMarkdownFences then CapToSentence; insight only gets .Trim(). If the model wraps insight in a backtick fence, raw delimiters reach the client. Fix: var insight = CapToSentence(StripMarkdownFences((content.Insight ?? string.Empty).Trim()), MaxInsightChars);

INFO: MCP get_daily_summary intentionally drops Insight (HabitTools.cs:518) -- confirmed per PR body.

What is good: Elegant bundling with one AI call, one cache entry, two fields. CompleteJsonAsync generalization is a clean improvement to the shared client. Four new focused tests cover happy path, 70-char trim, empty-insight degradation, and blank-summary failure. DailySummaryJson scoped private to AiSummaryService. Purely additive DTO change; no migration, no field removed.

Follow-ups (neither blocks merge): (1) Add [DistributedRateLimit("ai-summary")] to GetDailySummary and GetRetrospective. (2) Apply StripMarkdownFences to insight -- trivial one-liner.