Skip to content

fix(#644): reject floor-clamped velocity-1RM estimates so 80% scaling can't collapse to 2.2 lb/cable#645

Merged
9thLevelSoftware merged 2 commits into
mainfrom
fix/issue-644-velocity-floor-clamp
Jul 10, 2026
Merged

fix(#644): reject floor-clamped velocity-1RM estimates so 80% scaling can't collapse to 2.2 lb/cable#645
9thLevelSoftware merged 2 commits into
mainfrom
fix/issue-644-velocity-floor-clamp

Conversation

@9thLevelSoftware

@9thLevelSoftware 9thLevelSoftware commented Jul 10, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes #644.

AssessmentEngine.estimateOneRepMax extrapolates the velocity-based 1RM to a load and clamps the result to the 1.0 kg hardware floor. A "passing" estimate that sits at the floor is a diagnostic, not a usable baseline — the regression didn't actually reach the 1RM velocity, so 80% rounds to 0.8 kg and the routine collapses to ~2.2 lb/cable even when the user has a real stored-1RM or max-weight PR.

This patch rejects floor-clamped estimates at the shared velocity/baseline path so the fallback to stored-1RM / max-weight PR works for affected users.

Root cause

  1. AssessmentEngine.estimateOneRepMax clamps the extrapolated load to [1.0, 110] kg.
  2. VelocityOneRepMaxEstimator.estimate marks the result as passedQualityGate = true whenever the regression's R² is healthy, without checking the value.
  3. ComputeVelocityOneRepMaxUseCase persists that result unchanged.
  4. SqlDelightVelocityOneRepMaxRepository.getLatestPassing returns any row with passedQualityGate = 1, so a poisoned legacy row from a previous build short-circuits the resolver.
  5. ResolveRoutineScalingBaselineUseCase.resolveEstimatedOneRepMax takes any positive velocity value first and returns it, never falling through to stored-1RM / max-weight PR.
  6. RoutineExercise.resolveWeight applies 80% to 1.0 kg and rounds 0.8 kg back up to 1.0 kg → ~2.2 lb/cable.

Fix

Apply the floor-clamp guard at four shared points, all driven by a single VelocityOneRepMaxEstimator.isUsableEstimate predicate so the producer, repo, and resolver can't drift:

  • Producer — VelocityOneRepMaxEstimator.estimate: mark estimates at/below the 1.0 kg floor as passedQualityGate = false before returning. Future floor-clamped assessments never get persisted as passing.
  • Producer — ComputeVelocityOneRepMaxUseCase: skip persisting a result at/below the floor and return null (mirrors the existing distinctLoads<3 / r2<threshold early-returns).
  • Repository — SqlDelightVelocityOneRepMaxRepository.getLatestPassing / getAllPassing: filter floor-clamped rows out of the read path so users with a poisoned legacy row from a previous build get the fallback chain at runtime.
  • Resolver — ResolveRoutineScalingBaselineUseCase.resolveEstimatedOneRepMax: tighten the velocity guard to the shared predicate as a belt-and-suspenders check (a different repo impl plugged in here can't re-introduce the bug).

Regression coverage

  • VelocityOneRepMaxEstimatorTest:
    • estimate that lands on the 1 kg hardware floor is not passing — floor-clamped result is unpassing.
    • isUsableEstimate predicate matches the documented contract — boundary at 1.0 kg.
    • MIN_USABLE_ESTIMATE_KG equals 1 kg — pins the constant.
  • ResolveRoutineWeightsUseCaseTest:
    • ESTIMATED_1RM ignores floor-clamped velocity row and falls back to stored 1RM — 80% of 120 kg = 96 kg, not 1.0 kg.
    • ESTIMATED_1RM ignores floor-clamped velocity row and falls back to max-weight PR — 80% of 60 kg = 48 kg.
    • ESTIMATED_1RM still trusts a sane velocity row above the floor — guard does not strip legitimate estimates (100 kg → 80 kg at 80%).

Verification

  • ./gradlew :shared:compileKotlinMetadata -Pskip.supabase.check=true
  • ./gradlew :shared:compileTestKotlinIosArm64 -Pskip.supabase.check=true
  • ./gradlew :shared:testAndroidHostTest -Pskip.supabase.check=true ✅ (2426 tests, 0 failures; 4 new tests included)
  • ./gradlew :androidApp:testDebugUnitTest -Pskip.supabase.check=true
  • ./gradlew :androidApp:lintDebug -Pskip.supabase.check=true
  • ./gradlew :shared:verifyCommonMainVitruvianDatabaseMigration -Pskip.supabase.check=true

Out of scope (per GPT-5.5 RCA non-goals)

  • No Max-Weight / Max-Volume tab changes — the floor guard only affects ESTIMATED_1RM.
  • No new routine-editor special case — the existing ResolveRoutineScalingBaselineUseCase order is preserved; the velocity position is replaced with a stricter predicate instead.
  • No DB migration to delete legacy poisoned rows — the repo-layer filter handles them live.

… can't collapse to 2.2 lb/cable

AssessmentEngine extrapolates the velocity-1RM to a load and clamps the result to
the 1.0 kg hardware floor. A 'passing' estimate that sits at the floor is a
diagnostic, not a usable baseline — the regression didn't actually reach the 1RM
velocity, so 80% rounds to 0.8 kg and the routine collapses to ~2.2 lb/cable
even when the user has a real stored-1RM or max-weight PR.

Fix this once in the shared velocity/baseline path:

1. Producer (VelocityOneRepMaxEstimator): mark estimates at/below the floor
   unpassing so they never pass the quality gate to begin with.
2. Producer (ComputeVelocityOneRepMaxUseCase): skip persisting a result at/below
   the floor, returning null instead — mirrors what distinctLoads<3 / r2<threshold
   already do.
3. Repository (SqlDelightVelocityOneRepMaxRepository): filter floor-clamped rows
   out of getLatestPassing / getAllPassing so users with a poisoned legacy row
   from a previous build get the stored-1RM / max-weight PR fallback at runtime.
4. Resolver (ResolveRoutineScalingBaselineUseCase.resolveEstimatedOneRepMax):
   apply the same floor predicate as a belt-and-suspenders guard, so a different
   repo impl plugged in here can't re-introduce the bug.

Regression coverage:
- VelocityOneRepMaxEstimatorTest: floor-clamped estimate is unpassing; predicate
  boundary and constant pinned.
- ResolveRoutineWeightsUseCaseTest: floor-clamped row + stored-1RM → uses stored
  1RM (96 kg at 80%); floor-clamped row + max-weight PR → uses PR (48 kg at 80%);
  sane row above the floor is still trusted (100 kg → 80 kg at 80%).

Fixes #644
@kilo-code-bot

kilo-code-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Code Review Roast 🔥

Verdict: No Issues Found (incremental) | Recommendation: Merge

Incremental diff against 9fe412dd..1af2146 reviewed. All three findings from the previous pass are resolved in the new commit:

  • atFloor boolean inlined into the passed expression (VelocityOneRepMaxEstimator.kt:54).
  • 14-line OLS brainstorm comment collapsed to the contract: inputs chosen so load = (0.50-0.70)/-0.20 = 1.0 kg (VelocityOneRepMaxEstimatorTest.kt:64-65).
  • Repo filter moved to SQL via :minUsableEstimateKg parameter pushed before ORDER BY ... LIMIT 1, with a new androidHost regression test (VelocityOneRepMaxRepositoryTest.kt:13) proving a newer 1.0 kg poison row no longer hides the older usable passing estimate.

No new correctness, safety, or Ponytail findings on the incremental diff. The SQL filter now sits where it should (before LIMIT 1), the Kotlin predicate stays as a defensive belt-and-suspenders at the resolver, and the new test pins the exact regression scenario that motivated the move.

🏆 Best part: Pushing the predicate into the SQL WHERE instead of post-filtering LIMIT 1 is the correct fix for the older firstOrNull bug. Whoever moved the filter also added a real regression test for it — that's the kind of "prove you understood why the previous shape was wrong" follow-up that makes the next reader's life easier.

💀 Worst part: Nothing to roast in the increment. I checked.

📊 Overall: Like a good patch-2 — the author didn't just fix the typo, they fixed the reason the typo mattered. Lean already.

Files Reviewed (5 files in increment)
  • VelocityOneRepMaxEstimator.kt — atFloor inlined, no issues
  • VelocityOneRepMaxEstimatorTest.kt — comment collapsed, no issues
  • VelocityOneRepMaxRepository.kt — filter moved to SQL parameter, no issues
  • VitruvianDatabase.sq — added :minUsableEstimateKg WHERE clause on both queries, no issues
  • VelocityOneRepMaxRepositoryTest.kt — new androidHost test covering newer-floor / older-usable ordering, no issues

Correctness / Safety Findings

No correctness or safety findings.

Ponytail Review

Ponytail: Lean already. Ship.

Ponytail net: 0 lines.

Suggested Minimal Patch

No patch needed.

Final Merge Guidance

Can merge as-is. Previous review's findings are resolved; no new findings on the increment. The SQL filter fix (filter before LIMIT 1) is correct and accompanied by a regression test for the poison-row ordering scenario.

Previous Review Summary (commit 9fe412d)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 9fe412d)

Verdict: 2 nitpicks found | Recommendation: Merge (nitpicks are optional)

Overview

Severity Count
🚨 critical 0
⚠️ warning 0
💡 suggestion 0
🤏 nitpick 2
Issue Details (click to expand)
File Line Roast
VelocityOneRepMaxEstimator.kt 54 atFloor boolean is a one-shot relay runner — inline it
VelocityOneRepMaxEstimatorTest.kt 64 14 lines of math-brainstorming to explain a 1-line boundary test

🏆 Best part: The four-tier fix (producer × 2, repo × 2, resolver) all driven by a single isUsableEstimate predicate. That's not over-engineering — that's a producer/repo/resolver agreement document encoded in a 2-line function. Whoever wrote MIN_USABLE_ESTIMATE_KG as a documented constant and pinned it in tests clearly remembers the "rename a magic number at 3am" production outage. Belt-and-suspenders on the resolver, justified by the comment that another repo impl could be plugged in — that's the kind of defensive thinking that ages well.

💀 Worst part: The 14-line stream-of-consciousness math comment in VelocityOneRepMaxEstimatorTest. "Easier: feed points whose OLS gives a load below 0 at the supplied MVT." reads like the test author's TODO list, not documentation. Future readers need the contract, not the brainstorming session.

📊 Overall: Like a well-engineered bridge — slightly more scaffolding than strictly needed, but every bolt is load-tested and the lane markers glow in the dark. Ship it after a comment trim.

Files Reviewed (6 files)
  • VelocityOneRepMaxRepository.kt — 0 issues (defensive filter at the SQL/Kotlin boundary; LIMIT 1 keeps memory bounded)
  • VelocityOneRepMaxEstimator.kt — 1 nitpick
  • ComputeVelocityOneRepMaxUseCase.kt — 0 issues (mirrors existing null-early-return pattern)
  • ResolveRoutineScalingBaselineUseCase.kt — 0 issues (belt-and-suspenders guard is justified by the resolver not knowing which repo impl is wired)
  • VelocityOneRepMaxEstimatorTest.kt — 1 nitpick
  • ResolveRoutineWeightsUseCaseTest.kt — 0 issues (3 tests cover floor-clamped→stored-1RM, floor-clamped→PR, and sane-row-still-trusted)

Correctness / Safety Findings

No correctness or safety findings. The four-tier fix correctly prevents the floor-clamped estimate from short-circuiting stored-1RM / max-weight PR fallback, both for new assessments (producer+repo) and for poisoned legacy rows (repo+resolver). Tests cover all three fallback paths plus the sane-row sanity check.

Ponytail Review

  • VelocityOneRepMaxEstimator.kt:54: shrink — atFloor boolean is used once on the next line. Inline the check into the passed expression.
  • VelocityOneRepMaxEstimatorTest.kt:64: test-shrink — 14 lines of math-exploration comment to justify a boundary test. Replace with one line stating the chosen inputs and the resulting floor extrapolation.

Ponytail net: -13 lines (1 expression inlined, ~11 comment lines removed).

Suggested Minimal Patch

  1. Inline atFloor into the passed expression in VelocityOneRepMaxEstimator.kt.
  2. Replace the math-brainstorming comment in VelocityOneRepMaxEstimatorTest.kt with one line stating the inputs and floor outcome.

Final Merge Guidance

Can merge as-is. The two nitpicks are stylistic polish, not correctness. The fix itself is sound, well-tested, and appropriately defensive against legacy data.

Fix these issues in Kilo Cloud


Reviewed by minimax-m3 · Input: 42.2K · Output: 6.8K · Cached: 426.1K

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9fe412dd1d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@9thLevelSoftware 9thLevelSoftware merged commit 1843186 into main Jul 10, 2026
10 checks passed
@9thLevelSoftware 9thLevelSoftware deleted the fix/issue-644-velocity-floor-clamp branch July 10, 2026 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Routine 1RM% calculation falls through to ~2.2 lb/cable for cable-based exercises (Bent Over Row, Standing Cable Fly) despite valid PR data

1 participant