Add atomic streak gap repair (#442) - #505
Conversation
|
I verified that POST /api/gamification/streak/repair accepts no date fields, repairs only local yesterday, and consumes one freeze. I will add POST /api/gamification/streak/repair-gap with a dates array and the existing StreakInfoResponse success body. A separate StreakGapController keeps GamificationController and all existing single-day tests unchanged. The new command and FluentValidation validator will live under Orbit.Application/Gamification; UserStreakService and IUserStreakService will gain a separate evaluation method that verifies the complete contiguous gap ending local yesterday against persisted logs, freezes, and schedules. Existing access and monthly freeze limits remain applicable. Domain guards in StreakFreeze and User will validate the gap shape and consume the entire required balance in one operation. The handler will stage every freeze and the user update before one SaveChangesAsync, using the existing optimistic concurrency retry path. Insufficient balance will return a distinct stable conflict code without mutation. I rejected looping over the old command because separate saves could spend only part of the requested balance. I will add handler, validator, domain, service, and controller unit coverage, including two-day success, insufficient balance with a bank-count assertion, invalid selections, and concurrency failures. I will register the new person-initiated capability and regenerate architecture.json and architecture.html. No consumer changes belong to this ticket; thomasluizon/orbit-tickets#329 follows API delivery. |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed the current PR head, which contains only an empty preparation commit and has no implementation behavior to assess yet.
- Preparation commit
f3401e4changes no repository files; the planned streak gap repair remains future work.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Important
The repaired streak can make previously awarded milestones eligible again, allowing the next completion to refund the freezes that this operation spent.
Reviewed changes Reviewed the complete implementation added since the prior Pullfrog review.
- Added the gap repair API Introduced an authorized, rate limited
repair-gapendpoint with an explicit date selection and the existing streak response. - Implemented eligibility evaluation Validated the local yesterday boundary, contiguous scheduled dates, persisted completions and freezes, the preceding streak, monthly limits, and a strictly improved repaired state.
- Made persistence atomic Consumed the complete bank cost, staged every freeze row plus the user state, and saved once under the existing optimistic concurrency retry behavior.
- Expanded safeguards and coverage Added application validation, domain guards, stable conflict codes, agent capability metadata, generated API artifacts, and focused tests across the new layers.
GPT Sol ｜ 𝕏
|
I will fix only the freeze award cursor regression. First I am running RepairStreakGapCommandHandlerTests unchanged, then I will extend the Bank(2) success case through the next completion and observe the refund failure before changing production code. The cursor can be cleared in a prior saved recalculation, so capturing it inside the repair handler is too late. User.cs will retain the actual pre-reset cursor together with the last active date of that run before SetStreakState clears it. A narrow gap restoration method will restore that saved value only for the matching gap and clear the saved state. RepairStreakGapCommand.cs will use that method. The EF migration and generated model files are required to preserve this state across requests; a transient field would lose it when the user is reloaded. I will not derive awards from the restored streak or change the ordinary reset or award rules. Tests will cover the spent bank through streak 20, a new award at 21, repeated recalculation, a different run, and actual cursor values that differ from rounded streak length. The real UserStreakService unit test will drive recalculation, repair, and subsequent logged completions. After a successful build and focused projects I will commit, run the broader suite, push, and update this existing PR with the observed red and green evidence. |
There was a problem hiding this comment.
Important
Existing users whose streak dropped before deployment can still regain the freezes spent on a repair because the new persisted cursor fields start null for them.
Reviewed changes Reviewed the award cursor preservation added since the prior Pullfrog review.
- Persisted pre-gap award history Added nullable user fields that capture the award cursor and last active date when recalculation lowers a streak.
- Restored the matching run's cursor Routed gap repair through a domain method that restores saved award history only when its preceding date matches the repaired run.
- Expanded regression coverage Added domain, handler, service, and persistence tests for subsequent awards, full banks, unrelated runs, and entity reloads.
GPT Sol ｜ 𝕏
|
Fix round approach:
|
|
@pullfrog review |
|
This run was cancelled 🛑 The workflow was cancelled before completion. Please check the link below for details. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed the legacy cursor protection, generated artifact refresh, and base synchronization added since the prior Pullfrog review.
- Protected predeployment rows. Derived the last completed seven day freeze milestone when a repaired user has no matching persisted pre-gap cursor.
- Proved the legacy regression. Added domain and persistence coverage for null cursor columns and verified that the next completion cannot reaward a restored milestone.
- Refreshed architecture artifacts. Regenerated the architecture map so the final test references match the implementation.
- Synchronized the base. Merged the current main branch without changing the authoritative PR diff.
GPT Sol ｜ 𝕏
The only conflict was the committed architecture map, which is generated and byte-compared, so two branches touching any source file collide there and no side is meaningful. Regenerated from the merged tree rather than hand-resolved. This is the systemic defect #470 records, and it is why the UI repository stopped committing that map. orbit-api still commits it, so the collision remains here. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B
|
@pullfrog review |
There was a problem hiding this comment.
Important
The gap repair path treats calendar adjacency as streak adjacency, so valid gaps for weekly and every N day schedules cannot be repaired.
Reviewed changes Reviewed all 24 changed files and 7 commits at d72e975b, including the API, domain, persistence, schedule evaluation, tests, generated artifacts, and the current mobile consumer contract.
- Gap repair endpoint Added an authorized, rate limited
repair-gaproute with an explicit date selection and the existing streak response. - Eligibility and atomic spending Evaluated selected dates against local today, schedules, completions, freezes, monthly limits, and bank balance before staging one save.
- Award cursor persistence Added nullable pre-gap cursor fields, restoration logic, and a migration to prevent repaired milestones from being awarded twice.
- Coverage and metadata Added focused handler, validator, domain, controller, and service tests, plus capability, OpenAPI, and architecture updates.
- Consumer contract Confirmed the response DTO remains unchanged and the additive endpoint is sequenced API first; the current consumer has no paired gap repair feature PR yet.
GPT Sol ｜ 𝕏
Streak continuity runs over SCHEDULED occurrences, never calendar days: a weekly
habit's streak survives the six unscheduled days between two occurrences. Three
places asserted calendar adjacency instead, so a weekly or every-N-day user could
never repair a gap at all. The day before the gap is not a scheduled occurrence,
so it carries neither a completion nor a freeze, and the check always refused.
The rule moves to where the schedule is actually known:
- UserStreakService walks the ordered scheduled occurrences. The selection must
be an unbroken run of them, so a caller still cannot omit a missed occurrence
inside the gap, and the PRECEDING SCHEDULED occurrence must be completed or
frozen.
- StreakFreeze.CreateGap drops calendar-consecutiveness. That entity cannot see
a schedule, so it was rejecting valid gaps before one was ever loaded. It
keeps what a date list alone can prove: real dates, no duplicates, and a last
date of local yesterday.
- RepairStreakGapCommandValidator likewise checks duplicates only.
Red first, on the half that carries the defect. Reverting just the preceding
occurrence lookup to `gapStart.AddDays(-1)` fails the new weekly case; restored,
the suite is 23/23.
The second new case keeps the widening honest: when the prior week's occurrence
was ALSO missed, one selected date is not a whole gap and repair stays refused.
Schedule-awareness widens which gaps are contiguous; it never waives the
precondition.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B
|
@pullfrog review |
There was a problem hiding this comment.
Important
Sparse schedule repairs discard their persisted freeze award cursor because the handler still passes a calendar predecessor into schedule aware restoration.
Reviewed changes Reviewed all 24 changed files and 8 commits at c3bc9e6d, including the API, domain, persistence, schedule evaluation, tests, generated artifacts, and current mobile consumer contract.
- Gap repair API Added an authorized, rate limited endpoint with explicit date selection and the existing streak response contract.
- Schedule aware eligibility Evaluated the selected gap against local today, ordered scheduled occurrences, completions, freezes, and monthly limits.
- Atomic spending and award history Staged the bank deduction, repaired state, freeze rows, and persisted pre-gap cursor fields in one save with optimistic concurrency retries.
- Coverage and generated artifacts Added focused application, domain, controller, and service tests together with OpenAPI and architecture updates.
- Consumer sequencing Confirmed the response DTO is unchanged, the endpoint is additive, and no paired mobile gap repair PR is currently open, consistent with API first delivery.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Important
Sparse schedule repair remains incomplete: some valid supported cadences are still unavailable, and successful sparse repairs can suppress an earned freeze milestone.
Reviewed changes Reviewed the schedule-awareness fix added since the prior Pullfrog review.
- Relaxed boundary validation Removed calendar-consecutive checks from the validator and domain factory while retaining duplicate and local-yesterday guards.
- Moved completeness into scheduling Compared the selection with an ordered slice of the user-wide scheduled-date union and checked the prior scheduled occurrence.
- Added sparse-schedule coverage Added positive and negative weekly regressions for a predecessor seven calendar days before the gap.
GPT Sol ｜ 𝕏
…dary Three defects from the same root as the previous commit: continuity is defined over scheduled occurrences, and three more places still assumed calendar days. 1. THE AWARD CURSOR. The handler passed `gap[0].UsedOnDate.AddDays(-1)` to RestoreStreakAfterGapRepair while the service had already computed the real scheduled predecessor. On a sparse cadence those differ, the cursor match in User.RestoreStreakAfterGapRepair failed, and the derived-cursor fallback marked a newly crossed seven-day milestone as awarded WITHOUT granting its freeze, permanently suppressing the reward. The predecessor now travels on UserStreakState instead of being re-derived a layer up, the same one-source shape the readiness fallback needed. 2. THE YEARLY BOUNDARY. A yearly occurrence on yesterday has its predecessor 366 days before the gap, but history loaded only MaxStreakLookbackDays = 365. The predecessor fell outside the window, so the gap was index 0 and every yearly and every-365-day gap was refused. The window widens by MaxScheduleSpanDays, the longest span between two consecutive occurrences of a supported cadence, so it stays bounded. The GAP itself is still confined to the ordinary streak window; only its predecessor may sit in the widened margin. 3. Three tests pinned the calendar rule the previous commit removed. Each is updated rather than deleted, because the behaviour they guard survives: a calendar-sparse selection is still refused and still spends no freeze. It is now refused by the schedule-aware evaluation rather than the domain factory, which is exactly the relocation that makes a real weekly or yearly gap repairable. Full orbit-api suite: 6150 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B
|
@pullfrog review |
There was a problem hiding this comment.
Important
The widened history window truncates recent schedule generation for old habits, making valid repairs unavailable.
Reviewed changes Reviewed the schedule boundary and cursor restoration fix added since the prior Pullfrog review.
- Carried the scheduled predecessor. Added the evaluated predecessor to
UserStreakStateand passed it through the handler to award cursor restoration. - Widened predecessor history. Extended gap evaluation by one annual cadence span and added focused yearly and weekly schedule regressions.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Important
The expanded history window drops the recent schedule of established habits, and the annual success state does not survive the ordinary recalculation path.
Reviewed changes Reviewed all 26 changed files and 9 commits at 9bf1e555, including the API, domain, persistence, schedule evaluation, tests, generated artifacts, prior review fixes, and current mobile consumer contract.
- Gap repair API Added an authorized, rate limited endpoint with explicit date selection and the existing streak response contract.
- Eligibility and atomic spending Validated complete scheduled gaps, persisted activity, monthly limits, and bank balance before staging one save under optimistic concurrency.
- Award history persistence Added nullable pre-gap cursor fields, restoration logic, legacy fallback behavior, and the corresponding migration.
- Sparse schedule fixes Replaced calendar adjacency with ordered scheduled occurrences, carried the scheduled predecessor, and widened history for annual boundaries.
- Coverage and generated artifacts Added handler, validator, domain, controller, and service tests together with OpenAPI, architecture, capability, and consumer contract checks.
GPT Sol ｜ 𝕏
The previous commit widened the schedule request to reach a yearly predecessor and broke ordinary habits doing it. GetStreakScheduledDates caps a generated range at MaxRangeDays, so one 731-day request truncated the RECENT end: for a habit created before the widened start, the gap ending yesterday fell outside the generated dates and a valid repair reported unavailable. My yearly fixture hid it because that habit starts AT its predecessor, so effectiveFrom moved forward and the truncation never fired. Two bounded queries now, unioned. Each is inside the cap, so nothing is lost at either end. The yearly case then failed for a different reason, found by probing the helper rather than by reasoning about it: both queries returned exactly the right dates, but ComputeStreakAsOf still walked from the recent bound, so the predecessor that MAKES the repair worth anything sat outside the walk. The repaired streak scored no gain over the current one and the repair was refused as useless. The walk now starts at the earlier bound, which is where the schedule now starts. With the union in hand the predecessor is simply scheduled[gapStartIndex - 1] again, so the second lookup branch is gone. Regression added, per the required outcome: a daily habit created 800 days ago, older than the widened start, still repairs a gap ending yesterday. Full orbit-api suite: 6151 passed, 0 failed. One unrelated accountability test flaked once mid-session and passes 3/3 in isolation and in the two runs either side of it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B
|
@pullfrog review |
There was a problem hiding this comment.
Important
The new commit fixes its targeted schedule truncation defect without introducing another issue, but earlier blocking feedback remains unresolved, so this review does not approve the PR.
Reviewed changes Reviewed the schedule window split and regression added since the prior Pullfrog review.
- Split schedule generation Generated the recent streak window and predecessor margin separately before unioning them, preserving both sides under the shared range cap.
- Added an old habit regression Modeled a long running daily habit created before the widened start and verified that a gap ending yesterday remains repairable with the correct predecessor.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Important
The widened predecessor window now inflates repaired streak state for established habits and still leaves accepted annual repairs unstable under normal recalculation.
Reviewed changes Reviewed all 26 changed files and 10 commits at 6ba3ee4, including the latest split window fix and the full API, domain, persistence, scheduling, test, generated artifact, and consumer contract surface.
- Gap repair API Adds an authorized and rate limited endpoint with explicit dates and the existing streak response contract.
- Eligibility and atomic persistence Validates the complete scheduled gap, bank balance, monthly allowance, and prior activity before saving the user and freeze rows together.
- Award cursor durability Persists pre-gap award state, restores only a matching run, and supplies a fallback for rows created before the migration.
- Schedule aware follow ups Replaces calendar adjacency with scheduled occurrences, carries the scheduled predecessor, and splits schedule generation into bounded windows.
- Coverage and contracts Adds focused unit coverage, capability metadata, OpenAPI output, architecture artifacts, and confirms that the additive endpoint is sequenced API first for the mobile consumer.
GPT Sol ｜ 𝕏
Reverts the widened history window. Widening only the repair path made
eligibility and representation disagree, and the review caught the consequence:
- EvaluateGapRepairAsync accepts the repair on widened history and spends a
freeze;
- the handler builds its response through GetStreakInfoQuery, which calls
CalculateStateAsync on the ORDINARY 365-day window;
- that window sees yesterday's freeze without the preceding completion, so it
computes a zero streak;
- a later RecalculateAsync persists that zero, after the freeze was spent.
The user pays a freeze and gets nothing. Refusing a repair is strictly better
than accepting one that cannot survive the next recalculation.
So: ONE window, the engine's own, and the invariant written down. Eligibility is
decided over exactly the history that can represent the result. Weekly and
every-N-day gaps, which is what this ticket asked for, work entirely inside it.
Yearly gaps stay refused, exactly as they were before this PR, so nothing
regressed. The test now pins WHY rather than asserting they work, and the fix
belongs to the engine's window: filed as thomasluizon/orbit-tickets#483 with the
two traps recorded, including that GetStreakScheduledDates caps a per-habit range
at MaxRangeDays so any future widening must query in bounded slices.
MaxScheduleSpanDays goes with the approach that needed it.
Full orbit-api suite: 6151 passed, 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B
|
@pullfrog review |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed the canonical window correction added since the prior Pullfrog review.
- Aligned eligibility with canonical calculation. Removed widened predecessor history so repairs are accepted only when subsequent query and recalculation paths can represent the restored streak.
- Made annual boundary refusal explicit. Replaced the unsafe annual acceptance case with a regression confirming that gaps beyond the 365 day engine window are refused.
- Preserved supported sparse repairs. Retained weekly and every N day predecessor handling together with the established long running daily repair case.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Important
The new cursor persistence can suppress an earned freeze after a same-day log toggle.
Reviewed changes Reviewed all 25 changed files and 11 commits at c4bfde51, including the API, scheduling, domain state, persistence, tests, generated artifacts, and mobile contract boundary.
- Gap repair API: Adds an authorized, rate limited endpoint with explicit local-date selection and the existing
StreakInfoResponsecontract. - Eligibility and atomic persistence: Validates the complete scheduled gap, prior activity, monthly allowance, and bank balance before saving the user and freeze rows together.
- Award cursor durability: Persists pre-gap award state, restores a matching run, and derives a fallback for rows without a saved cursor.
- Schedule fixes and coverage: Uses the canonical streak window, carries the scheduled predecessor, rejects unsupported annual gaps, and adds focused application, domain, controller, and service tests.
- Consumer sequencing: Keeps the response DTO unchanged and the endpoint additive; the current mobile shared snapshot has no gap-repair consumer yet, consistent with API-first delivery.
GPT Sol ｜ 𝕏
The Roslyn ORBIT0001 gate caught my own violation: a five-line `//` block narrating why calendar-consecutiveness is absent. Only XML-doc or a WHY note linking an upstream URL is allowed. The rationale is worth keeping, so it becomes a summary plus remarks on the method rather than being deleted. Verified the way CI builds it: `dotnet build --no-incremental` is clean with zero ORBIT diagnostics. The earlier local `dotnet test` runs never recompiled this file against the analyzer, which is why CI saw it first. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B
|
@pullfrog review |
There was a problem hiding this comment.
Important
The legacy cursor fallback can mark a newly reached seven-day milestone as awarded without granting its freeze.
Reviewed changes Reviewed all 25 changed files and 12 commits at 996006b, including the API, domain state, persistence, scheduling, tests, generated artifacts, and mobile contract boundary.
- Gap repair API: Adds an authorized and rate limited endpoint with explicit local-date selection and the existing
StreakInfoResponsecontract. - Eligibility evaluation: Validates the complete scheduled gap against user-local today, canonical streak history, completions, freezes, predecessor activity, and monthly limits.
- Atomic persistence: Spends the full bank cost, restores streak state, stages every freeze row, and saves once under optimistic concurrency retries and unique-conflict handling.
- Award cursor durability: Adds nullable pre-gap cursor fields, migration coverage, matching-run restoration, and a fallback for rows created before deployment.
- Coverage and metadata: Adds application, validator, domain, controller, and service tests together with capability, OpenAPI, and architecture updates.
- Consumer sequencing: Keeps the response DTO unchanged and the endpoint additive; the current mobile repository has no paired gap-repair PR, consistent with the stated API-first delivery plan.
GPT Sol ｜ 𝕏
Two award-cursor defects, both of which silently cost the user a freeze. 1. A LATER DECREASE OVERWROTE THE SNAPSHOT. SetStreakState captured the pre-gap cursor on EVERY decrease, so log, unlog and relog of today after missing yesterday replaced (7, Sep 4), which identifies the repairable gap, with (0, Sep 6), which describes the break itself. Repair's exact match then failed and the derived cursor skipped a milestone that was never granted. Only the FIRST decrease captures now, and the pair is captured together so the date can never belong to a different run than the streak beside it. 2. THE LEGACY FALLBACK ROUNDED THE WRONG STREAK. With no matching snapshot, which is every pre-migration row, the cursor came from the FULL repaired streak, so milestones crossed by completions AFTER the gap were recorded as already awarded and their freezes were never issued. A row with six completions before the gap and one after reached 7, spent its banked freeze, and could never receive the one it had just earned. The fallback is now bounded by the streak going INTO the gap, which the service computes as of the scheduled predecessor and carries on UserStreakState. The cursor cannot advance past what was earned before the gap, so a newly crossed milestone stays awardable. `RestoreStreakAfterGapRepair` takes preGapStreak as a required parameter rather than an optional one: every caller should have to say what was earned before the gap, and there is exactly one production caller. Full orbit-api suite: 6153 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B
|
@pullfrog review |
There was a problem hiding this comment.
Important
The behavioral fixes are sound, but the prior cross-layer regression requirements remain incomplete: the new tests directly mutate User instead of carrying the failure sequences through UserStreakService and RepairStreakGapCommandHandler.
Reviewed changes Reviewed the cursor repair changes in 47295c6 since the prior Pullfrog review.
-
Preserved the first break snapshot. Prevented later streak decreases from replacing the cursor and activity date that identify the repairable gap.
-
Bounded the legacy fallback. Carried the streak at the validated scheduled predecessor into repair so milestones crossed after the gap remain awardable.
-
Added focused aggregate coverage. Added regressions for repeated decreases and a post-gap milestone on a row without a saved snapshot; the focused Domain, Application handler, and Infrastructure suites pass.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Important
Gap repair can lose earned awards, restore an invalid cursor, and spend against stale eligibility. These data-integrity failures need correction before merge.
Reviewed changes Reviewed all 25 changed files and 13 commits at 47295c63, including the API, domain state, persistence, scheduling, generated artifacts, tests, and consumer contract boundary.
- Gap repair API: Adds an authorized and rate limited endpoint with explicit local-date selection, stable conflict codes, and the existing
StreakInfoResponsesuccess contract. - Schedule-aware eligibility: Validates the complete scheduled gap against user-local today, canonical streak history, completions, freezes, predecessor activity, and monthly limits.
- Atomic spending and cursor durability: Stages the bank deduction, repaired user state, freeze rows, and persisted pre-gap award cursor in one save with optimistic retries and unique-conflict handling.
- Coverage and metadata: Adds application, validator, domain, controller, and service tests together with capability, OpenAPI, migration, and architecture updates.
- Consumer sequencing: Keeps the response DTO unchanged and the endpoint additive; the current mobile shared snapshot has no gap-repair consumer, consistent with the stated API-first delivery plan.
GPT Sol ｜ 𝕏
The review asked for these regressions to run through UserStreakService and the handler rather than mutating User directly. One of the two cannot honestly be written that way, and the first attempt at it was a test that could not fail. The legacy-fallback case IS now handler-level: six completions before the gap, one after, a pre-migration row with no snapshot and a banked freeze, driven through RepairStreakGapCommandHandler. It proves the milestone crossed after the gap stays awardable. The capture-once case stays on the entity, and the test says why. Driving it through RecalculateAsync needs an increase followed by a decrease, and this fixture cannot produce one: measured, Unlog leaves the recalculated streak at 1 rather than returning it to 0, so the service never sees a second decrease. The service-level version therefore passed with the fix REVERTED and proved nothing, which is worse than no test because its name claimed coverage it did not have. Red first, both, against the same revert: ASecondDecrease_KeepsTheSnapshotFromTheFirstBreak FAIL LegacyRowCrossingAMilestoneAfterTheGap_StillEarnsItsFreeze FAIL restored: 28 passed BuildRepairHandler is extracted now that a third caller exists, and the existing milestone theory uses it instead of building the same eight substitutes inline. Full orbit-api suite: 6153 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B
|
@pullfrog review |
There was a problem hiding this comment.
Important
Gap repair can still lose an earned freeze, suppress future awards with a stale cursor, and charge against stale schedule eligibility. The latest test-only commit leaves all three production defects present.
Reviewed changes Reviewed all 25 changed files and 14 commits at 1ebd2d52, including the API, domain state, scheduling, persistence, tests, generated artifacts, and consumer contract boundary.
-
Gap repair API: Adds an authorized and rate-limited endpoint with explicit user-local dates, stable conflict codes, and the existing
StreakInfoResponsesuccess contract. -
Schedule-aware eligibility: Validates complete scheduled gaps against canonical streak history, completions, freezes, predecessor activity, monthly limits, and bank balance.
-
Atomic mutation: Stages the bank deduction, repaired user state, and all freeze rows in one EF save with user optimistic-concurrency retries and duplicate-conflict handling.
-
Award cursor durability: Adds persisted pre-gap cursor fields, matching-run restoration, a legacy fallback, and the corresponding migration.
-
Coverage and metadata: Adds application, validator, domain, controller, and service tests together with capability, OpenAPI, and architecture updates.
-
Consumer sequencing: Keeps the response DTO unchanged and the endpoint additive; the current mobile shared contract has no gap-repair consumer yet, consistent with API-first delivery.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Important
No new defect was introduced by this test-only delta, but existing data-integrity feedback remains unresolved, so this review does not approve the PR.
Reviewed changes Reviewed the test coverage changes added in 1ebd2d5 since the prior Pullfrog review.
-
Strengthened legacy fallback coverage. Replaced the direct aggregate-only check with a six-completion legacy-row scenario driven through streak evaluation and the repair handler.
-
Centralized handler fixture wiring. Extracted the repeated repair handler setup into one helper while preserving the staged-freeze behavior used by existing tests.
-
Clarified repeat-decrease coverage. Renamed the first-snapshot regression and documented why the current substitute-based fixture keeps that assertion at the entity boundary.
GPT Sol ｜ 𝕏
Two more cursor defects, both mine. 1. REPAIR MADE A MILESTONE ELIGIBLE AND NOTHING GRANTED IT. Restoring the cursor only clears the way; the response is built by GetStreakInfoQuery, which calls CalculateAsync rather than RecalculateAsync, so an award earned by the repaired run sat pending and was lost the next time the streak reset. The repair now grants it in the same save that spends the bank. My earlier test hid this by calling AwardStreakFreezeIfEligible by hand, which production never does; it now asserts the freeze is actually banked. 2. THE SAVED CURSOR OUTRANKED THE RUN IT DESCRIBED. The snapshot deliberately survives later recalculations, so it can describe a LONGER run than the one being repaired: unlog an older pre-gap completion and a cursor of 14 still matches on date while the run is now 7, suppressing awards the shorter run has re-earned. The saved value is capped by what the current pre-gap run supports. And a hazard in my own design, caught by two existing tests going red: PreGapStreak defaulted to 0, so any caller that did not supply it read as "the run into the gap was zero", collapsing the cursor and re-granting milestones the user already had. It is nullable now. Unknown means DO NOT BOUND, which is the behaviour that stood before the bound existed, rather than silently meaning zero. Full orbit-api suite: 6154 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B
There was a problem hiding this comment.
Important
The new commit fixes its targeted award and cursor defects without introducing another high-severity issue, but earlier blocking consistency and regression feedback remains unresolved, so this review does not approve the PR.
Reviewed changes Reviewed the production and regression changes in 4530bf1 since the prior Pullfrog review.
-
Persisted earned awards. Granted milestones first crossed by the repaired run before the existing single save, keeping the award atomic with bank spending and freeze creation.
-
Bounded matching cursors. Capped a matching saved award cursor by the milestone supported by the evaluator's current pre-gap streak.
-
Preserved unspecified state. Made
PreGapStreaknullable so existing three-fieldUserStreakStatecallers do not accidentally treat an unknown run as zero. -
Strengthened focused regressions. Asserted the legacy handler path's bank and cursor immediately after repair and added an aggregate regression for a saved cursor above the current run.
GPT Sol ｜ 𝕏
CI rejected an eight-line narration block in User.cs. The rationale is worth keeping, so it moves onto the preGapStreak parameter documentation, where it also reaches anyone reading the signature rather than only someone inside the method. A correction worth recording, because I got this wrong twice tonight: the ORBIT Roslyn analyzers DO NOT run in the local `dotnet build`. A clean local build, even `--no-incremental`, proves nothing about ORBIT0001. Only CI catches it, and only for the Orbit.Domain project, which is why identical `//` comments in Orbit.Application and Orbit.Infrastructure pass. Check Domain comments by eye or push and read CI; do not trust a green local build for this rule. Full orbit-api suite: 6154 passed, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01Uhu7gcHwQ2n4V1fx6yZ71B
CI failed the Build on ten ORBIT0001 diagnostics in `Orbit.Application`, not in `Orbit.Domain`: seven in `RepairStreakGapCommand` and three in `RepairStreakGapCommandValidator`. The analyzer covers every project except migrations and generated files, so the project is not what made the difference. A clean local build proves nothing here, and now there is a measurement for why. `dotnet build` on this tree emits `warning CS9057`: the analyzer assembly references compiler version 5.9.0.0 while the local SDK runs 5.6.0.0, so Roslyn DISABLES the analyzer and the build reports 0 errors on the exact file CI then rejects. Locally the ORBIT rules do not run at all, in any project. `NoCommentsAnalyzer.IsAllowed` accepts a multi-line comment that starts with `/**`, so the three narration blocks keep every word and change only their form. Nothing was deleted: each one records a defect this pull request fixed, and a sweep of the whole diff for added `//` lines outside `///` now returns nothing. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_012LguAmjM5WjvsPonsJh4tJ
The previous commit fixed `Orbit.Application` and CI then failed on nineteen more diagnostics in `Orbit.Infrastructure`. The build stops at the first failing project, so fixing one only reveals the next, and my first sweep was truncated by a `head -30` and reported clean when it was not. This is the whole sweep, run to completion. `Directory.Build.props` attaches the analyzer to EVERY project except the analyzers themselves, tests included, so the pass covers `src/` and `tests/` together: five blocks in `UserStreakService` and four in `StreakGapRepairTests`. Grepping the complete repository for an own-line `//` outside `///`, a URL note, a migration and generated output now returns nothing. Every word is kept. `NoCommentsAnalyzer.IsAllowed` accepts a multi-line comment that starts `/**`, so only the form changed, and the converter refuses a block containing `*/` rather than silently ending the comment early. Local evidence and its limit: `dotnet build` reports 0 errors on this tree and proves nothing about ORBIT0001. Every project emits `warning CS9057`, because the analyzer assembly references compiler 5.9.0.0 while the local SDK runs 5.6.0.0, so Roslyn disables it. The rules do not run locally at all, in any project. Only CI can confirm this. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_012LguAmjM5WjvsPonsJh4tJ
|
@pullfrog review |
There was a problem hiding this comment.
Important
Gap repair can still spend banked freezes using schedule or completion eligibility that changed after evaluation, so the consistency boundary must be fixed before merge.
Reviewed changes Reviewed the complete 25-file, 18-commit gap-repair implementation at 4c1efc75, including the API contract, schedule evaluation, domain award state, migration, save path, generated artifacts, tests, and current mobile consumer boundary.
- Gap repair API Adds an authorized, rate-limited endpoint with explicit user-local dates, stable conflict responses, and the existing
StreakInfoResponsesuccess contract. - Schedule-aware evaluation Validates the complete scheduled gap against canonical streak history, completions, existing freezes, predecessor activity, monthly limits, and bank balance.
- Atomic domain update Stages the full bank cost, repaired state, award cursor, any newly earned freeze award, and every repair freeze row in one EF save with user concurrency retries and duplicate handling.
- Persistence and compatibility Adds nullable pre-gap cursor columns with legacy fallback behavior while keeping the response DTO unchanged and sequencing the additive consumer work after the API.
- Coverage and artifacts Adds focused application, validator, domain, controller, and service tests together with capability metadata, OpenAPI output, and architecture artifacts.
GPT Sol ｜ 𝕏
…stale Pullfrog raised this three times and was right every time. I resolved the first two claiming it was pre-existing and architectural. That premise is false, and two commands disprove it: git log --oneline origin/main -S "EvaluateGapRepairAsync" -- src/ no output git cat-file -e origin/main:.../RepairStreakGapCommand.cs absent Both the evaluation and the handler are introduced by THIS pull request, so the read-then-write hazard is created here and cannot be deferred to a ticket. The defect: EvaluateGapRepairAsync decides from the habits, the completion logs, the freezes and the schedule derived from them, and the save then spends the freeze bank. Those were two separate snapshots. `User.xmin` cannot hold them together, because a schedule-only habit edit commits WITHOUT touching the user row, so the optimistic token this command already retries on never fires. A cadence or due-date change landing between the two left a freeze spent on a date the committed schedule no longer accepts, with the bank charged all the same. The boundary already existed and this handler simply never joined it. HabitCeilingLock takes a transaction-scoped per-user advisory lock, and UpdateHabitCommand, LogHabitCommand, CreateHabitCommand, MoveHabitParentCommand and RestoreHabitCommand all hold it. The repair now runs inside HabitCeilingLock.ExecuteAsync, so evaluation and the spend happen in one transaction under the same key every writer whose output it reads must take. The user is loaded inside the lock too: a load taken before it would carry a pre-lock snapshot of the bank into a post-lock decision. The regression asserts ORDER rather than presence, because presence proves nothing about the window: lock, then evaluate, then save, with the key read from HabitCeilingLock.ForUser itself so a rename cannot leave it passing against a lock nobody else holds. Wiring the transaction exposed six failing end-to-end cases in StreakGapRepairTests: a substituted IUnitOfWork returns null from the transaction wrapper unless the operation is invoked. Both sites now run it as the real one does, which is also what makes those cases exercise the real boundary. Ticket 484 stays open, scope corrected, for what this does not cover: writing the invariant down, sweeping other handlers that spend a bank from an unprotected Habit or HabitLog read, and deciding whether those tables should carry a concurrency token so the hazard is detectable rather than only preventable. Test evidence: dotnet test, 6155 passed, 0 failed Orbit.Analyzers.Tests 32 passed Orbit.Domain.Tests 582 passed Orbit.Application.Tests 3313 passed Orbit.Infrastructure.Tests 2228 passed Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_012LguAmjM5WjvsPonsJh4tJ
|
There was a problem hiding this comment.
Important
The transaction now orders the repair correctly, but not all schedule and completion writers participate in its advisory lock. The remaining stale eligibility race can charge a banked freeze for a repair invalidated by a concurrent habit mutation.
Reviewed changes
Reviewed the production consistency change and its accompanying tests added since the prior Pullfrog review.
- Moved repair under the shared transaction lock: Wrapped user loading, eligibility evaluation, bank spending, freeze staging, and persistence in
HabitCeilingLock.ExecuteAsync. - Added lock ordering coverage: Updated handler fixtures and asserted advisory lock acquisition precedes evaluation and save.
GPT Sol ｜ 𝕏
| * The user is loaded INSIDE the lock for the same reason: a load taken before it would carry a | ||
| * pre-lock snapshot of the bank into a post-lock decision. | ||
| */ | ||
| var repaired = await HabitCeilingLock.ExecuteAsync( |
There was a problem hiding this comment.
HabitCeilingLock does not provide the promised consistency boundary because several production handlers that change these eligibility inputs never acquire it. For example, DeleteHabitCommand can commit a habit soft-delete after evaluation but before this save, then recalculate afterward, leaving the repair's bank charge and freeze committed against a schedule that no longer exists.
Technical details
# The advisory lock does not cover every eligibility writer
## Affected sites
- `src/Orbit.Application/Gamification/Commands/RepairStreakGapCommand.cs:47-101` coordinates only writers that acquire the same per-user key.
- `src/Orbit.Application/Habits/Commands/DeleteHabitCommand.cs:39-50` commits a soft delete without this lock before running its separate user recalculation.
- `src/Orbit.Application/Habits/Commands/LogHabitCommand.cs:94-107,224-232` acquires the lock for unlog, but its normal log path persists an eligibility-changing `HabitLog` without it.
- `src/Orbit.Application/Habits/Commands/SkipHabitCommand.cs`, `BulkLogHabitsCommand.cs`, and `BulkSkipHabitsCommand.cs` also change schedule or completion inputs without the shared lock.
- `tests/Orbit.Application.Tests/Commands/Gamification/RepairStreakGapCommandHandlerTests.cs:68-96` proves only the repair handler's local ordering, so it remains green when a production writer bypasses the lock.
## Failure sequence
1. Repair acquires the advisory lock and evaluates a gap using habit H.
2. `DeleteHabitCommand` soft-deletes H and commits that habit-only save without changing `User.xmin`.
3. Repair spends the bank, stages freezes, and commits from the stale schedule.
4. Delete recalculates afterward, but it cannot refund the spent bank or undo the committed repair rows.
## Required outcome
- Put every persisted mutation that can change habit, completion, or freeze eligibility under the same per-user consistency boundary, or make repair detect those intervening changes and fully re-evaluate before commit.
- Add a controlled concurrency regression using at least one real production writer, such as delete or normal log, to prove it cannot invalidate eligibility between evaluation and spending.
The lock-coverage finding is correct, and this pull request is being held rather than mergedI verified every claim in So my previous commit message was wrong where it said The boundary I claimed does not exist. A concurrent Why it is not being fixed in this pull request tonightThe two mechanisms that would actually close it are both cross-cutting, and picking between them is a
A PARTIAL rollout is worse than neither: it would leave the code asserting a boundary that still does This path spends a user's banked currency. Merging it with a known race, unattended and overnight, is So the pull request stays open and the thread stays unresolved. Everything else on it is green: The decision waiting for you is which of the two mechanisms to take, recorded with its trade-offs in What this pull request did earnThe repair now runs inside |
1 participant
Closes https://github.com/thomasluizon/orbit-tickets/issues/442.
POST
/api/gamification/streak/repair-gapaccepts{"dates":["2026-09-04","2026-09-05"]}. It validates the complete contiguous gap ending the user's local yesterday, spends exactly one banked freeze per selected day in one save, and returns the existingStreakInfoResponsewith the restored streak. Insufficient balance returns HTTP 409 witherrorCode: INSUFFICIENT_STREAK_FREEZESand leaves the bank unchanged. Unavailable gaps return HTTP 409 withSTREAK_GAP_REPAIR_UNAVAILABLE; malformed selections return HTTP 400.The existing single-day route, request, command, validator, and tests are unchanged. Existing access, scheduled-day eligibility, and per-month usage limits apply to the new operation. Domain guards reject invalid date sets and insufficient bulk spending. The handler stages all freeze records and the user balance before one EF save, with the existing user concurrency token and retry behavior protecting concurrent spending. A unique conflict resets tracking and reports an unavailable gap without a partial save.
The new person-initiated capability is registered in the agent catalog. OpenAPI and architecture artifacts are regenerated. API delivery precedes the consumer action in thomasluizon/orbit-tickets#329.
The fix round protects the pre-deployment cohort whose
PreGapFreezeAwardStreakandPreGapLastActiveDatevalues are both null.User.SetStreakStatepreserves a known pre-gap cursor before resetting it. When that saved value is absent,RestoreStreakAfterGapRepairnow derives the cursor from the restored streak with the same completed seven-day milestone calculation used byAwardStreakFreezeIfEligible.AppConstants.StreakMilestoneTierscontains the separate social event tiers[7, 14, 30, 90, 100, 365]used byFriendFeedEmitter; it is not the freeze award cadence, so the fallback reuses the domain calculation instead.The earlier unresolved thread
PRRT_kwDORKgXhc6fu5uvis stale and was left unresolved as requested. At headd114b21c,src/Orbit.Application/Gamification/Commands/RepairStreakGapCommand.cs:54already calledRestoreStreakAfterGapRepair, andsrc/Orbit.Domain/Entities/User.cs:593restored the matching saved pre-gap cursor. The current implementation retains that path at those locations and adds the legacy null fallback atsrc/Orbit.Domain/Entities/User.cs:595.Validation
dotnet build Orbit.slnx --no-restore: succeeded with zero errors. Existing package, compiler and obsolete Firebase API warnings remain.dotnet test: 6,111 passed with zero failures or skips across analyzer, Domain, Application and Infrastructure projects.node tools/arch-map.mjsfollowed bygit diff --exit-code -- architecture.json architecture.html: passed. The refreshed artifacts are committed together in5a7aa722.git diff --check main...HEADpassed, and the changed C# files contain no bare narration comments.Test evidence
dotnet test tests\Orbit.Infrastructure.Tests\Orbit.Infrastructure.Tests.csproj --filter "FullyQualifiedName~StreakGapRepairTests.SavedCursor_SurvivesUserReloadBeforeRepair"passed 1 test. It covered a post-migration user with a saved cursor and did not represent both new columns being null.dotnet test tests\Orbit.Infrastructure.Tests\Orbit.Infrastructure.Tests.csproj --filter "FullyQualifiedName~StreakGapRepairTests.LegacyPersistedUserWithoutSavedCursor_DoesNotReawardRestoredMilestone"failed 1 test. The observed failure wasExpected user.AwardStreakFreezeIfEligible() to be False, but found True, proving that the next completion re-awarded an old milestone.dotnet test tests\Orbit.Infrastructure.Tests\Orbit.Infrastructure.Tests.csproj --no-restore --filter "FullyQualifiedName~StreakGapRepairTests.LegacyPersistedUserWithoutSavedCursor_DoesNotReawardRestoredMilestone|FullyQualifiedName~StreakGapRepairTests.SavedCursor_SurvivesUserReloadBeforeRepair"passed both tests.dotnet test tests\Orbit.Domain.Tests\Orbit.Domain.Tests.csproj --no-restore --filter "FullyQualifiedName~StreakGapTests"then passed all 17 tests.Assumptions
datesarray instead of widening the old empty request or accepting only a count, so the API can reject stale or non-contiguous selections.StreakGapControllerowns the appended route instead of adding another POST toGamificationController, preserving the existing controller and its unchanged single-day test contract.External interface evidence
The new conflict path calls the existing
DbUniqueViolationhelper, which readsDbException.SqlState. I inspected and instantiated the installed Npgsql 10.0.3 package, including its constructor parameter metadata andPostgresErrorCodes.UniqueViolationconstant. The real exception returned:{"SqlState":"23505","MessageText":"duplicate","Severity":"ERROR","InvariantSeverity":"ERROR"}Reproduce in PowerShell after package restore:
The package's XML metadata also declares
PostgresException.SqlState. Installed EF Core 10.0.11 metadata declaresAutoTransactionBehavior.WhenNeededas the default; repository inspection found no override.UnitOfWork.SaveChangesAsyncdelegates to the context, whose override delegates once to EF after timestamp updates. This operation uses that single save for the balance and every freeze, with the existing unique user/date index and user concurrency mapping; no migration is needed.