feat(gamification): #539 expose per-achievement progress

[thomasluizon]

Backend support for the #539 b5 right-rail "Próxima conquista" module: exposes real per-achievement progress so the rail can pick the closest-to-unlock locked achievement and show its own bar, instead of a placeholder.

What

• ProgressMetric enum + AchievementDefinition.Metric / ProgressTarget — achievement thresholds DRY'd to one source.
• AchievementProgressService computes current/target per achievement (7 queries fixed, N+1-safe).
• AchievementDto.ProgressCurrent / ProgressTarget added on BOTH /gamification/profile and /achievements (append-only, deploy-API-first — safe for old mobile clients).

Consumer (paired PR)

thomasluizon/orbit-ui-mobile b5 (feature/539-b5-apply-design) consumes these fields: shared Zod schema append + the web rail picks the closest-to-unlock locked achievement with its own bar (honest fallback: no bar if none is quantifiable).

Merge order: this API PR merges FIRST (deploy-API-first), then the UI PR.

All tests green both repos.

🤖 Generated with Claude Code

[thomasluizon]

Consumer PR (merge this API PR first, then it): thomasluizon/orbit-ui-mobile#560

[claude]

Code Review: PR #419 — feat(gamification): #539 expose per-achievement progress

Scope: PR #419 in thomasluizon/orbit-api
Recommendation: NEEDS WORK

Summary

This PR adds ProgressMetric/ProgressTarget to AchievementDefinition, a pure AchievementProgressCalculator, and a new AchievementProgressService that loads a user's metric values in a fixed 7 queries, then surfaces ProgressCurrent/ProgressTarget on AchievementDto from both /gamification/achievements and /gamification/profile. The refactor correctly DRYs 17 of 18 quantifiable achievements' thresholds into AchievementDefinitions, is well tested, additive/backward-compatible on the DTO, IDOR-safe, and introduces no N+1. One achievement — Cheerleader — was missed in that DRY-up and now serves a progress target that contradicts its actual unlock point.

Findings

Critical

None.

High

[HIGH] Cheerleader's declared progress target (25) doesn't match its actual unlock threshold (10)
· dimension: Correctness (#1)
· location: src/Orbit.Application/Gamification/AchievementDefinitions.cs:81 vs src/Orbit.Application/Social/Commands/SendCheerCommand.cs:32,120
· issue: This PR wires ProgressMetric.CheersSent + ProgressTarget = 25 into the Cheerleader definition (new(Cheerleader, "Cheerleader", "Send 25 cheers", ..., ProgressMetric.CheersSent, 25)), which AchievementProgressCalculator.Compute now serves live via AchievementDto.ProgressCurrent/ProgressTarget. But Cheerleader is actually granted by SendCheerCommand.AwardCheerleaderAsync, gated by its own independent private const int CheerleaderThreshold = 10; (if (priorCheerCount + 1 < CheerleaderThreshold) return;) — completely decoupled from AchievementDefinitions. Every other quantifiable achievement in this PR was refactored to route its grant condition through AchievementDefinitions.ProgressTarget via AchievementChecks.TargetFor (confirmed: WeekWarrior, GoalSetter/Overachiever/DreamMaker, SquadGoals, EarlyBird/NightOwl etc. all match their real grant thresholds) — Cheerleader is the one exception that was missed, and pre-PR this was a dormant, cosmetic-only mismatch (the description text "Send 25 cheers" was already wrong, but nothing consumed it numerically since Metric defaulted to None).
· risk: A locked user who has sent, say, 8 cheers sees ProgressCurrent=8, ProgressTarget=25 (32%) on both /gamification/achievements and /gamification/profile, then unlocks the achievement two cheers later — a materially misleading progress bar that directly contradicts this PR's own stated purpose (an accurate "closest-to-unlock" right-rail module, replacing a placeholder). This is doubly confirmed by the PR's own test suite pinning both contradictory numbers independently (AchievementDefinitionsTests.cs asserts target 25; SendCheerCommandTests.cs's TenthSentCheer_... pins the actual grant at 10) with no test cross-checking them against each other.
· fix: Either (a) make SendCheerCommand.AwardCheerleaderAsync read its threshold from AchievementDefinitions.GetById(AchievementDefinitions.Cheerleader)!.ProgressTarget instead of its own CheerleaderThreshold constant (matching the DRY pattern this PR establishes everywhere else), or (b) correct the definition's ProgressTarget (and the "Send 25 cheers" description text) to 10 to match the actual grant point. Either way, add a test that pins the two values against each other so they can't drift again.
· reference: PR body ("achievement thresholds DRY'd to one source"); rubric dimension 1 (Correctness)

[HIGH] FEATURES.md update required in thomasluizon/orbit-ui-mobile
· dimension: FEATURES.md parity (#14)
· location: FEATURES.md (orbit-ui-mobile repo root — not in this checkout)
· issue: Per the PR body, this API change backs a new right-rail "Próxima conquista" module in the paired orbit-ui-mobile PR (feature/539-b5-apply-design) — the achievements surface gains real progress bars instead of a placeholder, a materially changed user-facing feature surface.
· risk: If the paired mobile PR doesn't add/edit the Achievements/Gamification row in FEATURES.md (and keep its Gating/Platform/Locale columns accurate) to reflect the new progress-bar capability, the code-derived feature inventory goes stale.
· fix: Confirm the orbit-ui-mobile PR updates FEATURES.md accordingly — this cannot be verified from the orbit-api checkout.
· reference: rubric dimension 14; root CLAUDE.md FEATURES.md parity contract

Medium

None concretely actionable beyond the High findings above.

Low / Info

[INFO] New DTO fields are additive/optional — backward-compatible
· dimension: Contract drift + backward-compat (#11)
· location: src/Orbit.Application/Gamification/Queries/GetAchievementsQuery.cs:20-21 (AchievementDto.ProgressCurrent/ProgressTarget, both int? defaulting to null, appended at the end of the record)
· issue/note: Pure append, no rename/removal, no newly-required field. Per dimension 11's rule ("field added as optional → forward-compatible"), this is Info-only, not a break. Old mobile clients deserializing this response simply ignore the two unknown keys.
· reference: rubric dimension 11

Subagents

Agent	Verdict
security-reviewer	PASS — no IDOR (all new queries scoped to the authenticated user's own id via the already-loaded User entity), no secret/PII leak, no new logging, existing PayGate/pro-access checks run before the new progress load, no injection surface (parameterized LINQ predicates only)
contract-aligner	NOT VERIFIABLE (Zod side) — no orbit-ui-mobile checkout in this environment; C#-side classification confirms the DTO change is additive/optional (Info, not DRIFT). Flags the required packages/shared follow-up for the paired PR (see FEATURES.md finding above, same underlying cross-repo dependency).

Validation

Check	Result
Build (dotnet)	N/A — CI adaptation: Build / Unit Tests run as separate required checks on this PR
Tests (dotnet)	N/A — CI adaptation: Build / Unit Tests run as separate required checks on this PR

Deferred — N/A dimensions & files not verdicted

• Dimension 8 (DESIGN.md/AI-slop): N/A — no apps/* UI files in this diff.
• Dimension 9 (Parity web↔mobile): N/A — frontend-only dimension, does not fire on an orbit-api diff; no ui-mobile files here to compare.
• Dimension 10 (i18n): N/A — frontend-only dimension, does not fire on an orbit-api diff.
• Dimension 11 (Contract drift): partially deferred — the C#-side (additive/optional) is verified Info-only; the TypeScript Zod-schema side is NOT VERIFIABLE (no orbit-ui-mobile checkout in this environment).
• All 16 changed files (10 source + 6 test) were read and given a verdict — nothing changed was left unexamined. See file list in Subagents/Findings above.

What's good

• 17 of 18 quantifiable achievements had their grant thresholds correctly DRY'd from scattered magic numbers into AchievementDefinitions.ProgressTarget, verified line-for-line against every existing grant callsite (AchievementChecks, AcceptFriendRequestCommand, GamificationService) — genuinely eliminates a class of future threshold-drift bugs.
• AchievementProgressService.LoadAsync is careful about cost: it short-circuits the expensive time-of-day log scan when both EarlyBird and NightOwl are already earned, and skips habit-log queries entirely when the user has no habits — both behaviors are unit-tested (AchievementProgressServiceTests).
• Query shape is a fixed 7 round-trips regardless of achievement count (N+1-safe), matching the PR body's claim.
• New DTO fields are correctly ordered as optional trailing parameters — no accidental positional-argument break for any existing caller.
• Good unit-test coverage across the new calculator, service, and both query handlers, including edge cases (value-exceeds-target clamping, earned vs. locked, None-metric achievements).

Recommendation

Fix the Cheerleader threshold/target mismatch (pick one source of truth and add a regression test) before merge — as shipped, the new progress bar for this achievement will show numbers that contradict the actual unlock point, undermining the PR's own stated goal. Confirm the paired orbit-ui-mobile PR carries the required FEATURES.md update. No other blocking issues found; the security and contract review both pass.

[claude]

Automated Review — PR #419

Recommendation: Request changes

What changed since the last review

The prior review flagged a High finding: Cheerleader's ProgressTarget (25) didn't match its real unlock threshold (hardcoded 10 in SendCheerCommand.cs). Commit f29bdea fixes this correctly — AchievementChecks.TargetFor is now public and SendCheerCommand.AwardCheerleaderAsync reads the threshold from AchievementDefinitions instead of a separate constant, with a regression test guarding it. Confirmed fixed.

A deeper look at the new AchievementProgressService.cs — the piece that computes the current value behind each progress bar, not just the target — surfaced a new, distinct bug.

Findings

[High] CurrentStreak progress metric reads the wrong streak — can show a full bar for an achievement that isn't earned

• src/Orbit.Application/Gamification/Services/AchievementProgressService.cs:56 sources the streak progress value from user.CurrentStreak.
• user.CurrentStreak is an account-wide, union-of-habits streak: UserStreakService.RecalculateAsync (src/Orbit.Infrastructure/Services/UserStreakService.cs) builds it via HabitScheduleService.GetUnionScheduledDatesForStreak(contributingHabits, ...) — a day counts toward the streak if any qualifying habit was completed that day.
• But the actual grant condition for all 9 streak achievements (WeekWarrior, FortnightFocus, MonthlyMaster, QuarterChampion, Centurion, HalfYearHero, YearOfDiscipline, StreakTitan, StreakImmortal) is evaluated per individual habit: GamificationService.ProcessLoggedHabit (line 166) computes HabitMetricsCalculator.Calculate(habit, today, tz).CurrentStreak per habit and passes that into AchievementChecks.CheckConsistencyAchievements (line 229) — which only ever sees one habit's own streak, with no cross-habit union.
• Failure scenario: a user with 2+ daily habits who alternates completions between them (e.g. logs "Meditate" some days, "Read" other days, but always logs at least one habit daily) can have user.CurrentStreak climb past 7, 14, 30+ days, while neither individual habit has ever reached even a 7-day streak — so the achievement is never actually granted. AchievementProgressCalculator.Compute (Math.Min(value, target)) then reports ProgressCurrent == ProgressTarget (a 100% bar) for IsEarned == false. This directly contradicts the feature's purpose of showing accurate progress, and affects half of the quantifiable achievement catalog.
• Fix: source the streak metric from the same quantity that gates the achievement — e.g. the max of each habit's own HabitMetricsCalculator.Calculate(...).CurrentStreak — rather than the account-wide user.CurrentStreak. Add a regression test analogous to the Cheerleader fix (multi-habit user, alternating completions, asserts progress doesn't overstate an unearned streak achievement).

Not verifiable in this CI run

• FEATURES.md / consumer-repo parity (orbit-ui-mobile) — sibling repo not checked out here; the author's companion PR (orbit-ui-mobile#560) should be reviewed on its own.
• TypeScript/Zod contract drift — same reason. The C#-side DTO change (AchievementDto.ProgressCurrent/ProgressTarget, both nullable, additive) is not itself a breaking change.

What's good

• Cheerleader threshold bug from the prior review is correctly fixed with a regression test preventing future drift.
• AchievementProgressService.LoadAsync is cost-conscious: skips the time-of-day log scan once both EarlyBird/NightOwl are earned, skips habit-log queries when the user has no habits, and reuses the existing GamificationService windowing constants (400 days / 90 days) consistently.
• Pay-gate check runs before the new progress query in both handlers.
• [Authorize] present on the controller; all new queries are scoped to the authenticated user; no PII/token leakage in the new DTO fields.
• Solid unit test coverage on the new calculator/service/handlers otherwise.

Recommendation

Fix the CurrentStreak metric source in AchievementProgressService.cs so the displayed progress matches the actual per-habit grant condition, with a regression test. Everything else in this PR is solid.

[claude]

[High] This sources the streak progress value from user.CurrentStreak, which is the account-wide union-of-habits streak computed in UserStreakService.RecalculateAsync via HabitScheduleService.GetUnionScheduledDatesForStreak (a day counts if any habit was completed).

But the actual grant condition for all 9 streak achievements is per-habit: GamificationService.ProcessLoggedHabit computes HabitMetricsCalculator.Calculate(habit, ...).CurrentStreak per individual habit and passes that into AchievementChecks.CheckConsistencyAchievements.

A user alternating completions across 2+ daily habits can have user.CurrentStreak climb well past a threshold (e.g. 7 for WeekWarrior) while no single habit has ever reached that streak — so AchievementProgressCalculator.Compute's Math.Min(value, target) shows a 100%-full progress bar for an achievement that isn't (and may never be) earned.

Suggest sourcing this from the max per-habit streak (e.g. habits.Select(h => HabitMetricsCalculator.Calculate(h, today, tz).CurrentStreak).DefaultIfEmpty(0).Max()) so the displayed metric matches the real per-habit gating condition, plus a regression test.

[claude]

Code Review: PR #419 (feat(gamification): #539 expose per-achievement progress)

Recommendation: NEEDS WORK

Summary
This PR wires a real per-achievement progress bar through the gamification API: a new ProgressMetric enum plus AchievementDefinition.ProgressTarget centralize the achievement thresholds, a new AchievementProgressService loads a user current metric values in a fixed set of about 7 queries, and AchievementDto gains two additive int? fields consumed by both /gamification/profile and /achievements. Engineering quality is high overall -- the diff already folds in two follow-up fixes (per-habit max streak instead of the union streak, and reading the Cheerleader threshold from its own definition) and ships matching unit tests for every new class. Adversarial verification (an independent skeptic pass over the code) surfaced one High-severity correctness gap that this PR own new field assignments create: two of the achievements it wires progress bars for, StreakTitan and StreakImmortal, can never actually reach 100 percent, because the underlying per-habit streak calculator structurally caps below their targets. Security and the API-side contract shape are both clean.

Findings

Critical: None.

HIGH -- StreakTitan/StreakImmortal progress bars target thresholds the streak calculator can never reach

• dimension: Correctness
• location: src/Orbit.Application/Gamification/AchievementDefinitions.cs:61 and :87 (this diff assigns ProgressMetric.CurrentStreak with target 500 and 1000); root cause in src/Orbit.Application/Habits/Services/HabitMetricsCalculator.cs:69 and :87; acknowledged but not resolved by this PR own comment at src/Orbit.Application/Gamification/Services/AchievementProgressService.cs:49
• issue: HabitMetricsCalculator.GenerateDayFilteredDates and GenerateFrequencyBasedDates build the expectedDates list with a while loop capped at 365 iterations, regardless of habit age or log history. CalculateCurrentStreak increments streak at most once per entry in expectedDates, so the returned CurrentStreak can never exceed 365. AchievementProgressService.LoadAsync maxCurrentStreak (added by this PR) is habits.Max of HabitMetricsCalculator.Calculate(...).CurrentStreak, so it inherits the same 365 ceiling -- and so does the actual grant path (GamificationService.ProcessLoggedHabit calling AchievementChecks.CheckConsistencyAchievements), which is the only production call site that can grant StreakTitan (target 500) or StreakImmortal (target 1000). A skeptic pass confirmed no other grant path exists and that these two achievements predate this PR; this diff only attaches ProgressMetric/ProgressTarget to them.
• risk: This diff is what turns a previously invisible bug into a directly user-visible, actively misleading one. Once the paired mobile PR ships, a Pro user chasing StreakTitan/StreakImmortal will see a progress bar that climbs to a hard ceiling around 73 percent / 36.5 percent and never completes, no matter how long the real streak runs. 750 plus 1500 XP of Legendary-tier content is dead content today, and this PR is the first thing that actively surfaces that to end users as broken-looking progress.
• fix: Scoped to this PR, either (a) raise HabitMetricsCalculator 365-iteration cap to scale with the callers needs, which also fixes the underlying grant-path bug for good, or (b) if that calculator change is out of scope here, do not assign ProgressMetric.CurrentStreak plus a target above 365 to StreakTitan/StreakImmortal in this PR -- leave them ProgressMetric.None (no bar) until the calculator is fixed in a follow-up, matching the honest-fallback principle already used for PerfectWeek/PerfectMonth.
• reference: rubric dimension 1 (Correctness); orbit-api CLAUDE.md No workarounds -- root-cause every bug

MEDIUM -- Third AchievementDto construction site skips the new progress fields

• dimension: SOLID / clean architecture (pattern inconsistency)
• location: src/Orbit.Application/Gamification/Commands/ReportEventCommand.cs:30
• issue: GetAchievementsQueryHandler and GetGamificationProfileQueryHandler both route AchievementDto construction through the new AchievementProgressCalculator.Compute helper; ReportEventCommandHandler.Handle builds a third AchievementDto directly and never calls it, leaving ProgressCurrent/ProgressTarget at their null default.
• risk: Currently harmless -- AchievementEventMap.ClientReportable only maps to ShowOff and YearInReview, both ProgressMetric.None, so Compute would also return null/null here. But the moment a future ClientReportable entry maps to a quantifiable achievement, this call site will silently under-report a just-earned achievement progress, and nothing will catch the inconsistency since it compiles fine against the optional trailing params.
• fix: Route this handler through AchievementProgressCalculator.Compute as well, so all three AchievementDto construction sites share one code path for the progress fields.
• reference: rubric dimension 3 (SOLID / clean architecture -- DRY at the right level)

Subagents

• security-reviewer: PASS -- Authorize intact on GamificationController, UserId sourced only from the JWT principal (no IDOR), all new queries are parameterized LINQ (no raw SQL), AchievementProgressService.LoadAsync is a fixed set of about 7 queries independent of achievement count with the time-of-day scan windowed to 90 days and skipped once both time-of-day achievements are already earned, no PII beyond counts exposed.
• contract-aligner: MATCH on the API side / NOT VERIFIABLE on the mobile side -- AchievementDto only appends two new optional trailing int? fields; all nine prior fields are unchanged in name, type and order, so this is forward-compatible for any already-shipped client. The paired orbit-ui-mobile repo is not checked out in this CI job, so the claimed Zod-schema append on the consumer side could not be independently confirmed.

Validation

• Build (dotnet): N/A -- CI runs Build/Unit Tests/SonarCloud as separate required checks; this review skips that phase per the CI wrapper instructions.
• Tests (dotnet): N/A -- same as above.

Deferred

• Dimension 8 (DESIGN.md/AI-slop): N/A, no apps/* UI files in this diff.
• Dimension 9 (Parity web/mobile) and Dimension 10 (i18n): N/A, frontend-only dimensions that do not apply to an orbit-api diff.
• Dimension 11 (Contract drift), mobile side: not verifiable in CI, orbit-ui-mobile is not checked out in this job.
• Dimension 14 (FEATURES.md parity): deferred/N/A from the api side -- this diff adds no new screen/route/tool/gating change of its own, only two additive fields on two existing endpoints; the actual user-facing feature (the right-rail progress module) ships in the paired orbit-ui-mobile PR, which is the appropriate place for any FEATURES.md update.
• All 18 changed files (11 source, 7 test) were read and given a verdict; nothing was skipped for size.

What is good

• The diff already incorporates two forward-fix commits addressing exactly the kind of correctness issues a careful review would flag: per-habit MAX streak instead of the union user.CurrentStreak, and the Cheerleader threshold reading from its own AchievementDefinition.ProgressTarget instead of a duplicated local constant.
• AchievementProgressService.LoadAsync windowing (400 days) is deliberately matched to GamificationService.AchievementLogWindowDays so displayed progress and actual grant timing stay consistent -- verified by reading both files side by side.
• Query shape is genuinely N+1-safe: a fixed, small number of queries regardless of achievement count, with the expensive time-of-day log scan skipped once both time-gated achievements are earned.
• Full unit-test coverage for every new type, including a dedicated test proving per-habit max, not union, streak is used.
• The two new AchievementDto fields are purely additive/optional -- safe for already-shipped mobile clients per the PR own deploy-API-first framing, confirmed independently by contract-aligner.

Recommendation
Fix, or scope around, the StreakTitan/StreakImmortal unreachable-target issue before merge -- it is the one finding with real user-facing impact once the paired UI PR ships, and this PR is squarely the place to either fix HabitMetricsCalculator cap or hold those two achievements at ProgressMetric.None until it is fixed. The Medium ReportEventCommand inconsistency is a fast fix but not blocking. Everything else -- security, contract shape, tests, query-shape discipline -- is solid and ready to ship as-is.

[claude]

Code Review: PR #419 — feat(gamification): #539 expose per-achievement progress

Scope: PR #419 in thomasluizon/orbit-api
Recommendation: NEEDS WORK

Summary

Well-executed DRY refactor: every achievement now carries a Metric/ProgressTarget, and a new AchievementProgressService computes live per-achievement progress in a fixed 7-query, N+1-safe pass, surfaced via AchievementDto.ProgressCurrent/ProgressTarget on /gamification/profile and /achievements. This revision also folds in fixes for the Cheerleader threshold mismatch and the long-streak/high-volume unreachable-target issues flagged in earlier review passes on this PR. One new correctness bug was introduced by the latest fix commit (11f495a, "streak progress reflects max per-habit streak, not the union"): the streak-progress aggregation now pools bad-habit streaks into the same metric used for consistency achievements, which the actual grant path explicitly excludes bad habits from.

Findings

Critical

None.

High

[High] AchievementProgressService.LoadAsync pools bad-habit streaks into the CurrentStreak progress metric — a progress bar can show 100%/ready for an achievement that can never actually be granted from that habit

• dimension: 1. Correctness
• location: src/Orbit.Application/Gamification/Services/AchievementProgressService.cs:50-52
• issue: maxCurrentStreak = habits.Max(h => HabitMetricsCalculator.Calculate(h, today, userTimeZone).CurrentStreak) has no !h.IsBadHabit filter. HabitMetricsCalculator.CalculateCurrentStreak (src/Orbit.Application/Habits/Services/HabitMetricsCalculator.cs:109-142) computes the opposite semantics for a bad habit — consecutive non-logged (abstinence) days, breaking the moment the habit is logged. The real grant path never awards CurrentStreak-metric achievements (WeekWarrior, FortnightFocus, …) from a bad habit: GamificationService.ProcessLoggedHabit only calls AwardLoggedHabitXpAndAchievementsAsync (the sole caller of CheckConsistencyAchievements) when !habit.IsBadHabit (GamificationService.cs:170-173; the method's own doc comment at 205-211 confirms this is deliberate — a bad-habit log is "a slip, not progress").
• risk: A user abstaining from a bad habit for ≥7 consecutive expected days (with no equal/higher good-habit streak) sees e.g. "Week Warrior" render at 100%/ready in the new progress rail, while IsEarned stays false forever — directly contradicting this PR's own goal of an honest, accurate progress bar.
• fix: Exclude bad habits from the streak aggregation only (leave totalCompletions/EarlyLogs/NightLogs unfiltered — they already match their own unfiltered grant paths): habits.Where(h => !h.IsBadHabit).Max(...) with a zero-good-habits fallback to 0.
• reference: CLAUDE.md "No workarounds — root-cause every bug" / rubric dimension 1

[High] FEATURES.md update required in thomasluizon/orbit-ui-mobile

• dimension: 14. FEATURES.md parity
• location: sibling repo, not checked out here
• issue: This PR materially changes the achievements feature surface (real per-achievement progress data feeding a new right-rail UI module) — the rubric's trigger condition.
• risk: If the paired orbit-ui-mobile PR (feature/539-b5-apply-design) doesn't update FEATURES.md's achievements row, the feature inventory goes stale.
• fix: Confirm the paired mobile PR carries the FEATURES.md update before it merges. Not verifiable from this repo/CI job.
• reference: rubric dimension 14 (mandatory flag when sibling repo isn't checked out)

Medium

[Medium] Three independent constants encode the same "must exceed the largest ProgressTarget" invariant with no compiler-enforced link

• dimension: 3. SOLID/clean architecture (DRY)
• location: HabitMetricsCalculator.cs:10 (MaxStreakHorizonDays=1100), GamificationService.cs:39-40 (StreakLogWindowDays=1100, TotalCompletionWindowDays=2750), AchievementProgressService.cs:30-31 (same two, same values)
• issue: All three carry the identical WHY comment but nothing derives them from AchievementDefinitions.All.Max(a => a.ProgressTarget ?? 0) — this is the same bug shape this PR's history has already fixed twice.
• fix: Derive one shared constant (e.g. AchievementDefinitions.MaxProgressTarget) and reference it from all three.
• reference: CLAUDE.md "No workarounds — root-cause every bug"

[Medium] The Unstoppable-reachability fix has no regression test at the GamificationService level

• dimension: 13. Backend hard rules (tests)
• location: GamificationService.cs:136,233 vs. tests/Orbit.Application.Tests/Services/GamificationServiceTests.cs
• issue: The existing CheckVolumeAchievements_2499Completions_DoesNotGrantUnstoppable test only exercises the pure function, which never had this bug — the actual defect was the skip-gate wrapping the call inside GamificationService, changed from LegendaryVolume to Unstoppable. No test exercises a user with LegendaryVolume already earned crossing 2500 completions through ProcessHabitLogged.
• fix: Add a GamificationServiceTests case seeding earned = {Liftoff, LegendaryVolume}, total completions ≥2500, asserting Unstoppable is granted.
• reference: orbit-api CLAUDE.md "Testing" / tests/CLAUDE.md

Low / Info

None (signal gate).

Subagents

Agent	Verdict
security-reviewer	PASS — no Controller/[Authorize] surface changed; every new/changed query in AchievementProgressService and SendCheerCommand is scoped to user.Id/sender.Id/ids derived from the caller's own habits (verified: habits h.UserId == user.Id; logs via habitIds.Contains(l.HabitId) where habitIds derives only from the already-scoped habit list; goals g.UserId == user.Id; cheers c.SenderId == user.Id); AchievementChecks.TargetFor's InvalidOperationException only fires on compile-time-constant achievement ids that all have ProgressTarget set, so it's an unreachable static invariant, not an attacker-triggerable crash; no PII in the new DTO fields (plain ints); no new logging added.
contract-aligner	NOT VERIFIABLE — sibling repo not checked out. By inspection, AchievementDto's new fields are additive/optional (int? = null, appended at the end) — forward-compatible for old mobile clients.

Validation

Check	Result
Build (dotnet)	N/A — skipped per CI adaptation (separate required check)
Tests (dotnet)	N/A — skipped per CI adaptation (separate required check)

Deferred

• Dimension 8 (DESIGN.md/AI-slop) — N/A, no apps/* files.
• Dimensions 9/10 (Parity, i18n) — N/A, frontend-only.
• Dimension 11 (full Zod-schema field comparison) — needs sibling repo, not checked out.
• Phase 6 step 2 (cross-model second opinion) — scoped to Critical only; no surviving Critical this pass.
• Phase 7 (build/test) — skipped per CI adaptation.

All changed files received a verdict; nothing silently skipped.

Recommendation

Fix the bad-habit streak pooling in AchievementProgressService.LoadAsync before merge — this is a fresh regression from the PR's own latest commit, not a pre-existing issue. The two Medium items are good hygiene, not merge-blocking. Confirm the paired orbit-ui-mobile PR carries the FEATURES.md update.

[claude]

[High] Bad-habit streaks pool into the CurrentStreak progress metric, but the grant path excludes them

habits.Max(h => HabitMetricsCalculator.Calculate(h, today, userTimeZone).CurrentStreak) has no !h.IsBadHabit filter. For a bad habit, CalculateCurrentStreak computes the opposite semantics — consecutive non-logged (abstinence) days. But GamificationService.ProcessLoggedHabit only calls AwardLoggedHabitXpAndAchievementsAsync (the sole caller of CheckConsistencyAchievements) when !habit.IsBadHabit (GamificationService.cs:170-173) — a bad-habit's streak can never grant a CurrentStreak-metric achievement like WeekWarrior/FortnightFocus.

Result: a user abstaining from a bad habit for ≥7 days sees "Week Warrior" render at 100%/ready in the new progress rail, while IsEarned stays false forever.

Fix: habits.Where(h => !h.IsBadHabit).Max(...) with a zero-good-habits fallback to 0 (leave the other metrics — totalCompletions, EarlyLogs, NightLogs — unfiltered, they already match their unfiltered grant paths).

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #419 — feat(gamification): #539 expose per-achievement progress

Scope: PR #419 in thomasluizon/orbit-api
Recommendation: APPROVE

Summary

This PR exposes real per-achievement progress (ProgressCurrent/ProgressTarget) on AchievementDto for GET /achievements and GET /gamification/profile, backed by a new ProgressMetric enum + AchievementDefinition.Metric/ProgressTarget (one source of truth for thresholds) and a new AchievementProgressService that loads a user's metric values in a fixed, N+1-safe number of queries. The diff also folds in bugfixes surfaced while building this (per-habit vs. union streak, bad-habit abstinence exclusion, an off-by-achievement short-circuit that made StreakImmortal/Unstoppable unreachable, and the 365-day iteration cap in HabitMetricsCalculator that silently truncated long streaks). Correctness, security, and backward-compat all check out; two Medium findings flag threshold values this PR's own new ProgressTarget field duplicates elsewhere without wiring through the new single source of truth.

Findings

Critical

None.

High

None.

Medium

[Medium] Achievement threshold duplicated instead of wired to the new single source of truth (AchievementChecks.TargetFor)
· dimension: SOLID/clean architecture — DRY (CLAUDE.md rule 10)
· location: src/Orbit.Application/Gamification/Services/GamificationService.cs:278 (goalCount >= 3 for GoalSetter), :297 (completedGoals >= 5 for Overachiever), :300 (completedGoals >= 10 for DreamMaker); src/Orbit.Application/Social/Commands/AcceptFriendRequestCommand.cs:26,84 (SquadGoalsThreshold = 5 for SquadGoals)
· issue: This PR adds AchievementDefinition.ProgressTarget as "the single source of truth for a linear-threshold achievement" (AchievementChecks.TargetFor XML doc) and applies it to SendCheerCommand.cs's Cheerleader check (replacing a local CheerleaderThreshold constant with AchievementChecks.TargetFor(CheerleaderAchievementId)). The same pattern was not applied to GoalSetter (3), Overachiever (5), DreamMaker (10), or SquadGoals (5) — those still compare against hardcoded literals that now duplicate the ProgressTarget values declared in AchievementDefinitions.cs (3, 5, 10, 5 respectively).
· risk: The values match today, so nothing is broken yet. But this is the exact bug class this PR's own commit history already fixed once (see 9624475/1fa3c26/f29bdea): if a future change bumps one of these achievement's ProgressTarget (e.g. rebalancing SquadGoals to 10 friends) without also updating the hardcoded grant-side literal, the progress bar shown to users would silently diverge from the actual unlock threshold — displaying "10/10" while the achievement was already granted at 5, or vice versa.
· fix: Replace the four hardcoded literals with AchievementChecks.TargetFor(AchievementDefinitions.GoalSetter) / .Overachiever / .DreamMaker / .SquadGoals, mirroring the SendCheerCommand.cs change in this same PR, and drop AcceptFriendRequestCommand.SquadGoalsThreshold.
· reference: CLAUDE.md rule 10 (DRY at the right level); this PR's own AchievementChecks.TargetFor doc comment.

[Medium] Reachability window-day constants (1100 / 2750) triplicated as magic numbers instead of derived from AchievementDefinitions
· dimension: SOLID/clean architecture — DRY (CLAUDE.md rule 10)
· location: src/Orbit.Application/Habits/Services/HabitMetricsCalculator.cs:10 (MaxStreakHorizonDays = 1100), src/Orbit.Application/Gamification/Services/GamificationService.cs:39-40 (StreakLogWindowDays = 1100, TotalCompletionWindowDays = 2750), src/Orbit.Application/Gamification/Services/AchievementProgressService.cs:30-31 (TotalCompletionWindowDays = 2750, StreakLogWindowDays = 1100)
· issue: The same two numeric horizons (1100 days ≈ StreakImmortal's 1000-day target + buffer; 2750 ≈ Unstoppable's 2500-completion target + buffer) are hand-copied as separate private constants across three files, each with its own WHY comment restating "must exceed the largest target."
· risk: This is precisely the class of bug this PR fixes for the second time in its own commit history (1fa3c26, "make long-streak + high-volume achievements reachable" — the horizon was previously 365/400 days, silently capping StreakImmortal/Unstoppable as unreachable). Raising StreakImmortal's or Unstoppable's target again in AchievementDefinitions.cs alone (now the documented single source of truth for thresholds) would not automatically raise these three independent windows — reintroducing the same latent bug with no compiler or test signal, since nothing ties them together.
· fix: Derive the two windows once from AchievementDefinitions.All (e.g. AchievementDefinitions.All.Where(a => a.Metric == ProgressMetric.CurrentStreak).Max(a => a.ProgressTarget!.Value) + buffer) in one shared location (or a single named constant in AchievementDefinitions/AchievementChecks) that the other three consume, so a future target bump can't silently desync from the reachability window again.
· reference: CLAUDE.md rule 10 (DRY at the right level); prior fix commit 1fa3c26 on this same branch.

Low / Info

None posted (signal gate).

Subagents

Agent	Verdict
security-reviewer	PASS — no injection, tenant-isolation, data-exposure, or unhandled-exception issues. All new queries in AchievementProgressService scope to the authenticated user.Id; AchievementChecks.TargetFor() is only ever called with compile-time-constant achievement ids, never request-controlled input, so its throw path isn't a DoS vector.
contract-aligner	NOT VERIFIABLE — sibling orbit-ui-mobile repo not checked out in this CI job. The AchievementDto change is a trailing-optional-field addition (ProgressCurrent, ProgressTarget, both int? = null), which is forward-compatible / additive by construction (Info, not a break) regardless of the sibling schema's current state. The PR body documents a paired consumer PR (orbit-ui-mobile feature/539-b5-apply-design) that adds the matching Zod fields, consistent with the stated deploy-API-first merge order.

Validation

Check	Result
Build (dotnet)	N/A — CI runs Build as a separate required check; skipped here per the CI adaptation.
Tests (dotnet)	N/A — CI runs Unit Tests as a separate required check; skipped here per the CI adaptation.

Deferred — N/A dimensions & files not verdicted

• Dimension 8 (DESIGN.md/AI-slop) — N/A, no apps/* UI files in this diff (backend-only PR).
• Dimension 9 (Parity) — N/A, frontend-only dimension owned by the orbit-ui-mobile review; not verifiable from orbit-api.
• Dimension 10 (i18n) — N/A, no new user-facing strings introduced (the PT translation dictionary in GamificationService.cs is untouched).
• Dimension 11 (Contract drift) — sibling-schema side not verifiable in CI (see contract-aligner row above); API-side change judged additive/Info.
• Dimension 14 (FEATURES.md parity) — judged N/A: the diff adds no new screen/route/tab, Astra/MCP tool, gating change, platform-availability change, or locale-specific behavior — it adds progress metadata to the existing Achievements feature, consumed by a progress-bar UI enhancement in the paired orbit-ui-mobile PR. None of the rubric's concrete trigger examples match; flagging for the human reviewer to override if the paired PR's FEATURES.md update disagrees with this judgment.
• All 21 changed files (13 source + 8 test) were read and given a verdict; nothing was skipped for size.

What's good

• The two bundled correctness fixes (per-habit-not-union streak progress, and the LegendaryVolume→Unstoppable short-circuit fix that was silently capping the two highest-tier achievements as unreachable) are exactly the kind of real bug a "let's expose progress" feature should surface, and both ship with targeted regression tests (AchievementProgressServiceTests.LoadAsync_HighUnionStreakButLowPerHabitStreaks_ReturnsMaxPerHabitStreakNotUnion, AchievementProgressServiceTests.LoadAsync_BadHabitAbstinenceStreak_ExcludedFromStreakProgress).
• AchievementProgressService.LoadAsync is genuinely N+1-safe: a fixed ~7 queries regardless of achievement or habit count, with an explicit short-circuit (CountTimeOfDayLogsAsync) skipping the expensive time-of-day log scan once both Early Bird and Night Owl are earned — verified against FriendGraphService.CountAcceptedFriendsAsync (single query) too.
• The ProgressMetric/ProgressTarget addition is a real DRY win for the achievements this PR touched (streak + volume checks in AchievementChecks.cs, Cheerleader in SendCheerCommand.cs) — the two Medium findings above are about extending that same win to the achievements it didn't touch.
• Good test coverage across the new pure calculator, the new DB-backed service, and both query handlers, including boundary cases (value exceeding target gets clamped, earned achievements report a full bar, None-metric achievements stay null).

Recommendation

Approve as-is. The two Medium findings are pre-existing-pattern inconsistencies this PR's own new ProgressTarget field surfaces (not regressions it introduces, and today's values are consistent) — worth a fast follow-up so the "single source of truth" the PR advertises actually covers every quantifiable achievement, not just the ones this diff happened to touch.