Raise habit ceiling to abuse guard - #490
Conversation
|
Approach:
Expected files are the named application and content files, a new migration with its generated metadata and model snapshot updates if required by the repository pattern, plus the existing focused test files that cover those paths. |
![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 The complete diff for 55dcfaa was reviewed; this revision contains no code, configuration, test, or contract changes.
- Placeholder commit
55dcfaamatches itsmainparent exactly and only establishes the branch for the planned ORB-186 work. - Behavioral scope There is no implementation to assess yet; implementation commits will need a fresh review when they land.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Important
The shared ceiling is not atomic under concurrent creation, and its refusal still carries an entitlement code that redirects users to the upgrade screen.
Reviewed changes I reviewed the implementation added since the prior Pullfrog review at 55dcfaa.
-
Raised the fallback ceiling Changed the default live root-habit limit from 10 to 1000.
-
Applied one ceiling to both plans Removed the Pro bypass from direct creation and onboarding allowance trimming.
-
Migrated persisted configuration Updated the existing
FreeMaxHabitsrow and added migration operation coverage. -
Updated guidance and boundaries Reworded the assistant explanation and added free, Pro, bulk, onboarding, fallback, and refusal tests.
⚠️ The abuse guard is not atomic
At 999 live roots, two concurrent creates can both read 999 and then commit, yielding 1001; concurrent bulk requests can overshoot farther. The onboarding lock uses a different key and no other creation path shares it, so the new ceiling does not hold under adversarial concurrency.
Technical details
# Enforce the live root-habit ceiling atomically
## Affected sites
- `src/Orbit.Application/Common/PayGateService.cs:19-24` — counts live roots and returns before any mutation transaction or shared lock begins.
- `src/Orbit.Application/Habits/Commands/CreateHabitCommand.cs:51-118` — checks the count, then inserts independently.
- `src/Orbit.Application/Habits/Commands/BulkCreateHabitsCommand.cs:71-111` — checks before its transaction and can insert a full batch after another request passes the same count.
- `src/Orbit.Application/Habits/Commands/DuplicateHabitCommand.cs:34-63` — checks and inserts with no shared serialization boundary.
- `src/Orbit.Application/Chat/Tools/Implementations/CreateHabitTool.cs:152-176` — repeats the same uncoordinated check and insert.
- `src/Orbit.Application/Profile/Commands/ApplyOnboardingCommand.cs:92,144-150` — serializes only onboarding requests under `onboarding-apply:{userId}`, which does not contend with the other creation paths.
## Required outcome
- Make the configured live root-habit ceiling invariant across concurrent direct, bulk, duplicate, chat, and onboarding creation.
- Add a concurrent creation unit test that starts below the boundary and proves the committed live-root count cannot exceed the configured ceiling.GPT Sol ｜ 𝕏
|
Resolved the review findings in f92cbd4. Habit creation now uses one per-user transaction advisory lock across direct, bulk, duplicate, chat, and onboarding paths, with a deterministic concurrent boundary test. The ceiling refusal is now a neutral failure with no entitlement error code. |
There was a problem hiding this comment.
Important
The shared lock still excludes paths that reactivate completed roots, so the 1000 live-habit invariant remains bypassable under concurrency.
Reviewed changes I reviewed f92cbd4, which was pushed after the prior Pullfrog review at 771c6f1.
- Serialized creation callers Wrapped direct, bulk, duplicate, chat, and onboarding creation with one per-user transaction advisory lock.
- Neutralized ceiling refusals Replaced
PAY_GATEwith a generic failure and updated assertions so clients no longer treat the abuse ceiling as an upgrade prompt. - Added concurrency coverage Added deterministic lock serialization coverage and caller lock assertions.
⚠️ Reactivations bypass the new serialization boundary
Completed root unlogs and end-date reactivations still perform a count followed by a later mutation without acquiring habit-ceiling:{userId}. At 999 live roots, a reactivation can pass alongside a locked creation or another reactivation and both can commit, leaving 1001.
Technical details
# Serialize every path that increases the live root count
## Affected sites
- `src/Orbit.Application/Common/PayGateService.cs:260-291` — `HabitReactivationAllowance` checks `CanCreateHabits` and applies the transition without the shared lock.
- `src/Orbit.Application/Habits/Commands/LogHabitCommand.cs:117-158` — a completed-root unlog passes the count check and persists later.
- `src/Orbit.Application/Habits/Commands/UpdateHabitCommand.cs:77-135` — an end-date change can reactivate a root before a later save.
- `src/Orbit.Application/Chat/Tools/Implementations/UpdateHabitTool.cs:103-117` — the chat update path performs the same unlocked reactivation transition.
- `tests/Orbit.Application.Tests/Common/HabitCeilingLockTests.cs:12-52` — the new concurrency test covers only two operations that both acquire `HabitCeilingLock`.
## Required outcome
- Make every transition that increases the live top-level root count contend on the same per-user lock, with the lock held until persistence commits.
- Add a deterministic unit test that overlaps creation with reactivation and proves the committed live-root count cannot exceed the configured ceiling.GPT Sol ｜ 𝕏
|
@pullfrog review |
There was a problem hiding this comment.
Important
The 1000 live-root ceiling is still bypassable because several transitions that enter the counted set do not use the new allowance and serialization boundary.
Reviewed changes I reviewed the complete five-commit branch at 8c65f20, including the habit ceiling behavior, every habit state transition that affects the count, transaction semantics, migration, and tests.
- Universal abuse ceiling Raises the configured live top-level habit limit to 1000 for free and Pro users and returns a neutral failure at the boundary.
- Creation serialization Uses one per-user transaction advisory lock for direct, bulk, duplicate, chat, and onboarding creation.
- Persistence and guidance Migrates the existing configuration row, updates assistant guidance, and regenerates the architecture map.
- Coverage Adds caller lock assertions, deterministic create serialization coverage, migration assertions, and free and Pro boundary tests.
⚠️ The ceiling is attached to creators, not the counted state
The new lock prevents concurrent creates from overshooting, but other commands can add a live root without participating. At 1000 live roots, restoring an uncompleted deleted root or promoting an active sub-habit commits 1001 sequentially; completed-root reactivation also retains the concurrent check-then-mutate race.
Technical details
# Enforce the ceiling on every transition into the live-root set
## Affected sites
- `src/Orbit.Application/Habits/Commands/RestoreHabitCommand.cs:26-64` restores an uncompleted root without an allowance check or `HabitCeilingLock`.
- `src/Orbit.Application/Habits/Commands/MoveHabitParentCommand.cs:30-34` promotes an active sub-habit to a root without an allowance check or lock.
- `src/Orbit.Application/Habits/Commands/UpdateHabitCommand.cs:77-107` and `src/Orbit.Domain/Entities/Habit.cs:405-410` can clear completion through `IsGeneral` without recognizing that the root re-enters the counted set.
- `src/Orbit.Application/Common/PayGateService.cs:260-291`, `src/Orbit.Application/Habits/Commands/LogHabitCommand.cs:117-158`, and `src/Orbit.Application/Chat/Tools/Implementations/UpdateHabitTool.cs:103-117` check reactivation allowance without acquiring the shared transaction lock through persistence.
## Required outcome
- Any mutation that changes a habit from child, completed, or deleted into an uncompleted, undeleted root must check the configured ceiling while holding `HabitCeilingLock.ForUser(userId)` through save and commit.
- Mutations that remain outside the counted set must not consume a root slot.
- Cover sequential restore and child-to-root promotion at the ceiling, plus an overlap between locked creation and completed-root reactivation.
## Suggested approach
- Put the live-root entry invariant behind one shared application transaction boundary used by creation, restore, reparenting, and reactivation instead of maintaining a list of creator wrappers.GPT Sol ｜ 𝕏
|
Review fix approach: centralize live root entry behind the existing per user transaction helper so it acquires HabitCeilingLock, checks capacity only when the mutation enters the counted set, and holds the lock through persistence and commit. Route restore, child promotion, regular update reactivation, chat update reactivation, and completed root unlog through that boundary. Extend the entry predicate to include IsGeneral clearing completion, preserve mutations that remain deleted, completed, or children without consuming capacity, and add sequential ceiling coverage plus deterministic creation and reactivation overlap coverage. Files will stay within the reviewed application handlers, the shared ceiling helper, and their focused tests. |
There was a problem hiding this comment.
Important
The registered move_habit chat tool still bypasses the serialized ceiling and can promote an active child past the 1000-root guard.
Reviewed changes I reviewed the behavioral and generated changes added since the prior Pullfrog review at 8c65f20.
- Unified guarded transitions Added shared live-root entry helpers that check allowance under
HabitCeilingLock. - Covered commands and update tools Routed restore, parent promotion, completed-root unlog, end-date reactivation, and
IsGeneralreactivation through the guarded transaction boundary. - Expanded regression coverage Added sequential transition tests and an overlapping creation-versus-reactivation test.
- Regenerated architecture metadata Refreshed the architecture artifacts after the application and test changes.
⚠️ The live-root ceiling is still bypassable through move_habit
The incremental fix routes move_habit_parent through the guarded command, but move_habit remains registered and directly clears ParentHabitId without an allowance check or lock. At the ceiling, that path succeeds and the chat persistence step commits a 1001st live root.
Technical details
# Route every chat promotion through the live-root entry invariant
## Affected sites
- `src/Orbit.Application/Chat/Tools/Implementations/MoveHabitTool.cs:31-58` loads a tracked habit and promotes it directly with `SetParentHabitId(null)`.
- `src/Orbit.Infrastructure/Services/AgentCatalogService.Capabilities.cs:172-183` keeps both `move_habit` and the guarded `move_habit_parent` tool active in the habit-write capability.
- `src/Orbit.Application/Chat/Commands/ProcessUserChatCommand.Persistence.cs:12-18` later persists the direct tracked mutation.
- `src/Orbit.Application/Habits/Commands/MoveHabitParentCommand.cs:21-29` is the corrected path that checks `HabitLiveRootEntry.FromPromotion` while holding the shared lock.
## Required outcome
- Ensure every active chat tool that can promote a child either delegates to `MoveHabitParentCommand` or applies the same serialized live-root entry boundary.
- Refuse an active-child promotion at the configured ceiling without mutating or persisting the child.
- Add coverage for the registered `move_habit` surface, or remove that legacy surface if `move_habit_parent` replaces it.GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Important
The move_habit fix still reads transition state from an entity tracked before the advisory lock, so concurrent chat writes can promote an active root beyond the 1000 habit ceiling.
Reviewed changes I reviewed b180598, the delta since the prior Pullfrog review at 4772809.
- Guarded legacy moves Routed the registered
move_habittool throughMoveHabitParentCommandinstead of mutating the tracked entity directly. - Updated regression coverage Replaced direct-mutation tests with MediatR dispatch, failure propagation, tenant isolation, and ceiling-refusal assertions.
- Refreshed architecture artifacts Regenerated metadata to include the command dependency and updated test references.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes I reviewed 86108de, the delta since the prior Pullfrog review at b180598.
- Moved entity lookup behind the guard Removed the pre-dispatch tracked habit loads from both registered move tools, so
MoveHabitParentCommandnow classifies promotion state after acquiringHabitCeilingLock. - Preserved result metadata safely Added post-success
FindAsynclookups, which useAsNoTrackingand cannot pollute the command handler's tracked entity. - Aligned regression fixtures Updated the move-tool tests and shared tool factory to exercise command-first dispatch, failure propagation, and post-command display lookup behavior.
GPT Sol ｜ 𝕏
|
1 participant
Summary
Dependency
Do not merge until #168 is merged.
Testing
dotnet build Orbit.slnx --no-restore: 0 errors.dotnet test --no-build: 5,950 passed.FreeMaxHabitsto1000with the abuse-guard description.Assumptions
You've reached the 1000 habit limit.; rejected adding new explanatory copy.External interface evidence
The migration test reads Entity Framework Core 10.0.10
UpdateDataOperationafter schema introspection of the installedMicrosoft.EntityFrameworkCore.Relational.dll. The complete property schema returned was:Columns string[],ColumnTypes string[],IsDestructiveChange bool,IsReadOnly bool,Item object,KeyColumns string[],KeyColumnTypes string[],KeyValues object[,],Schema string,Table string, andValues object[,]. Re-derive it by loading the installed 10.0.10 Entity Framework Core and Relational assemblies and callingGetProperties()onMicrosoft.EntityFrameworkCore.Migrations.Operations.UpdateDataOperation.Closes #192
Ticket: ORB-186, migrated as #192 in the ticket repository, https://github.com/thomasluizon/orbit-tickets/issues/192