Skip to content

Add a strength parser; stop guessing unknown targetTypes (#60) - #62

Open
fabxyz wants to merge 1 commit into
cygnusb:mainfrom
fabxyz:feat/strength-template-parser
Open

fabxyz wants to merge 1 commit into
cygnusb:mainfrom
fabxyz:feat/strength-template-parser

Conversation

@fabxyz

@fabxyz fabxyz commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Closes #60. Follow-up filed as #61 (see Vocabulary below).

_parse_workout only knew the endurance targetType vocabulary and sent everything else to duration_seconds, so a 3×12 squat read back out of the library as "12 seconds" and 27.9 kg as intensity_low: 27900.

Implemented along the lines you specified, with the three defaults from my last comment (minus totalSets, as decided).

What changed

  • _parse_strength_workout / _parse_strength_exercise — reps, weight (kg / lbs / bodyweight / effort target), rest, and the fields a rewrite needs (origin_id, overview, sets).
  • _parse_workout keeps its endurance contract, and its existing tests are untouched as the regression fence. _parse_workout_header() holds the six shared fields; sets stays out of it and lives in the strength parser, per your call.
  • The catch-all else is gone. Unknown targetType, restType and weight unit surface as *_raw pairs rather than a guessed unit. That one line is what produced Add duration_open: manual/lap-press steps for workouts #59 and list_workout_templates reports strength reps as duration_seconds #60 both, so the next unknown type is now visible instead of silently wrong.
  • One wire-encoding table, module-level with both columns, referenced from _target_fields, _parse_workout and _parse_strength_exercise.

Your six, addressed

  • (a) Dispatch is per exercise, not per program. You were right that this matters: the program-level dispatch I first wrote mislabelled the strength steps of a hybrid template — the same bug one sport along. _parse_exercise(ex, program_sport) dispatches on the exercise's own sportType with the program's as fallback, and a test pins a strength step inside a sportType: 1 program.
  • (b) Wire types coerced. Confirmed your prediction — _unscale raised TypeError on a string intensityValue. New _as_number() coerces before dividing; intensityDisplayUnit is compared as a string since it goes out as "6" and comes back as 6.
  • (c) lbs with a missing intensityPercent recovers the pound value from the kg equivalent rather than reporting 0. This turned out to be the common case, not the edge one — see below.
  • (d) restType fallback is honest: 3 → 0, 1 → restValue, anything else → rest_type_raw / rest_value_raw.
  • (e) _EXERCISE_DROP no longer discards originId, intensityCustom, intensityDisplayUnit, isIntensityPercent. Rather than comment on the disagreement I removed it: a raw view that hides the fields the parsed view decodes is useless for debugging exactly the cases you'd reach for it.
  • (f) Table done, and server.py's "exactly one duration key" contract is updated with the code.

Vocabulary: taking the split, not the alias

I'm taking your second option, so this PR does not claim the round-trip property.

The load and structural keys (weight_kg, weight_lbs, rest_seconds, sets, origin_id, overview) already match the write side's input names. The target axis does not, and your correction was right that my original justification didn't survive it: save_strength_workout_template takes target_type / target_value, has no input for bodyweight (it spells that by omitting both weight keys, so feeding the key back works only because the builder ignores unknown keys), and none at all for effort_target. reps is a better name, not a matching one.

So the docs now say plainly that the output is not yet pasteable into the write tool, and #61 tracks closing it. The round-trip test asserts that every written value decodes back to what was typed — deliberately not that the names match; its docstring says which axis is which and why.

Also per your correction: re-scheduling an unmodified template already worked and always did — schedule_workout_template posts the raw library item back inline and never touches _parse_workout. Nothing here fixes that, and this PR doesn't claim to. What it fixes is that list_workout_templates reported strength templates wrongly, and what #61 would unlock is read → modify → write.

Evidence

Fixtures are the wire shapes of a real strength template built in the Coros app and read back on 2026-09-09: a 4×12 decline dumbbell bench press at 31 kg, a 4×6 greatest stretch at bodyweight, the same press in pounds, a 3×60s burpee with an effort target, and 4×10 bicycle crunches with rests skipped. Recorded in the test docstrings as in #59.

Worth reading the capture, because it corrected three guesses I'd taken from the write side:

  1. A real lbs exercise comes back with intensityPercent: 0. Only templates this server wrote carry the typed pounds there. My first draft read pounds out of that field and returned 0 lbs against real app data — your instinct to demand an app-built template, not just a sent→stored diff, is the only reason this didn't ship.
  2. intensityCustom: 1 is the app's bodyweight marker too, not just ours — only the companion value differs (app sends 0, we send ""). An exercise left at the app's default reads intensityValue: 0, intensityCustom: 0, intensityDisplayUnit: 0 and is a real 0.0 kg. Verified by flipping one exercise from that default to Bodyweight in the app and diffing the re-read: intensityCustom 0 → 1 was the entire change.
  3. intensityDisplayUnit: 0 with a non-zero value isn't a weight at all. It's the app's effort target on a 1–10 RPE scale, stored unscaled — the burpee shows "Target 5, Moderate" for intensityValue: 5. This is a fourth load type neither of us had in view; it reads correctly now but has no write input, which is part of Strength write tools should accept the read side's vocabulary (reps / duration_seconds / bodyweight / effort_target) #61.

One thing to flag from probing, since it affects how much the parser can assume: the server does not validate loads. Six deliberately invalid POSTs to /training/program/add — an effort target on a weight-only exercise, an effort target of 99, a kg weight on an exercise the app only offers effort for, a 1-second rest against the app's 3s floor — all returned "0000" and read back byte-identical. So the parser has to stay readable in the face of values no app screen can produce, and a successful write is not validation. The catalogue does carry a signal (of 382 entries, exactly 6 have no intensityValue key — the weightless ones, including the burpee), which is the other half of #61.

Checks

272 passed, ruff and mypy clean. The endurance parser's existing tests were not modified.

🤖 Generated with Claude Code

`_parse_workout` only knew the endurance targetType vocabulary and sent
everything else to `duration_seconds`, so a 3x12 squat read back out of
the library as "12 seconds" and 27.9 kg as `intensity_low: 27900`.

Strength is now its own namespace on the read side, mirroring the write
side: `_parse_strength_workout` / `_parse_strength_exercise` decode reps,
weight (kg/lbs/bodyweight/effort target), rest and the fields a rewrite
needs (origin_id, overview, sets). `_parse_workout` keeps its endurance
contract and its existing tests unchanged as the regression fence.

Dispatch is per EXERCISE, not per program: both builders emit
`hybridTotalSets`, so the wire admits mixed programs, and the strength
builder stamps `sportType: 4` on every exercise. Dispatching on the
program sport alone would reproduce this same bug on a hybrid template.

The root cause was a catch-all `else` reporting every unknown targetType
as seconds -- the line that produced cygnusb#59 and cygnusb#60 both. Unknown targetType,
restType and weight unit now surface as `*_raw` pairs instead of a guessed
unit, so the next unknown type is visible rather than silently wrong. The
targetType wire encoding for both namespaces is documented in one
module-level table, referenced from the read and write sides.

Evidence, per cygnusb#59: fixtures are the wire shapes of a real strength
template built in the Coros app and read back on 2026-09-09. That capture
corrected three guesses taken from the write side -- a real lbs exercise
returns `intensityPercent: 0` (pounds must be recovered from the kg
equivalent), `intensityCustom: 1` is the app's bodyweight marker while an
untouched default is a real 0.0 kg, and `intensityDisplayUnit: 0` with a
value is an effort target on the app's 1-10 RPE scale, not a weight.

Also: coerce numeric wire fields before dividing (they arrive as strings
on some paths, where `_unscale` raised TypeError), and stop dropping
originId/intensityCustom/intensityDisplayUnit/isIntensityPercent from the
raw view -- they are the semantic core of this parser, so hiding them
there defeated the escape hatch for exactly these cases.

Read output is NOT yet pasteable into save_strength_workout_template: the
target axis still differs (write takes target_type/target_value, read
returns reps/duration_seconds) and there is no write input for bodyweight
or effort_target. Deliberately left to a follow-up rather than widening
this PR; the docs say so plainly instead of claiming the round trip.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@fabxyz
fabxyz requested a review from cygnusb as a code owner September 10, 2026 01:11

@cygnusb cygnusb left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Verified against the branch: 272 passed, ruff and mypy clean locally, CI green on 3.11/3.12/3.13, one commit, no drift from main. Scope is what we agreed — server.py changes only the list_workout_templates docstring, the write tools are untouched, and #61 carries the vocabulary work.

The evidence discipline paid for itself. All three corrections the app capture forced are real findings that were not derivable from the write side, and the lbs one would have shipped as 0 lbs against every app-created template. Removing the fields from _EXERCISE_DROP rather than commenting on the disagreement is the right call.

Three things to fix, then this is good to merge.

1. Blocking: distance group headers are still read as steps

The isGroup check sits fourth in the chain (coros_api.py:891-905), behind targetType == 5. But the server normalizes a pure-distance repeat group's header onto exactly that type — live-verified 2026-08-12 and documented in this file at coros_api.py:1317-1329: sent as targetType=2, targetValue=0, stored as targetType=5, targetValue = one iteration's meters x100.

_parse_workout({"sportType": 1, "exercises": [
  {"name": "", "targetType": 5, "targetValue": 40000, "sets": 3, "isGroup": True, "groupId": "0"},
  {"name": "400m", "targetType": 5, "targetValue": 40000, "sets": 1, "groupId": "123"},
]})
# header -> {"sets": 3, "distance_meters": 400}   <- phantom step, no is_group / repeat

A 3x400m template therefore lists four distance steps instead of three plus a header, and its distance double-counts. Not a regression — the old catch-all was equally wrong here — but this PR introduces group handling and misses the one group shape the repo has already confirmed live.

Fix is the ordering: test isGroup first, then the targetType chain. Please add a test for the normalized distance header alongside the app-shaped one; the existing test covers only targetType=0 and only the time header, which is why this passes today.

2. A missing intensityValue key reads as bodyweight: True

coros_api.py:997 uses raw_value in ("", None), which also catches an absent key — while the comment two lines above says only the two confirmed spellings count. Confirmed are "" (this server) and intensityCustom: 1 (the app). A missing key is a third, unobserved spelling:

{"name": "T_warmup", "targetType": 2, "targetValue": 300,
 "intensityCustom": 0, "intensityDisplayUnit": 0}
# -> {"duration_seconds": 300, "rest_seconds": 0, "bodyweight": true}

By your own catalogue finding in #61, those are most likely the six weightless entries — warm up, cool down, rest, indoor rower, skierg, burpee. Those have no weight axis at all; they are not "bodyweight". Same principle the rest of the PR applies: keep the two confirmed spellings, let anything else fall through to the *_raw pair where it is visible.

3. The endurance contract did change — the docs say it didn't

The PR text and commit message both say _parse_workout keeps its endurance contract. It doesn't: a group header now returns is_group / repeat instead of sets plus a phantom duration_seconds: 0, and sub-steps gained group_id. That's an improvement, and the existing tests are silent on it only because they never covered group headers — so the regression fence didn't hold here, it just wasn't asked.

But the docs still promise the old shape: README.md:314 and server.py:817 both say endurance steps carry "exactly one duration key", which a header now doesn't, and is_group / repeat / group_id appear in neither. This is the same class of silent shape change the PR exists to eliminate — please document the three keys and adjust both contract sentences.

Smaller things

  • _LBS_IN_KG (coros_api.py:813) duplicates _LB_TO_KG (:1727) — same constant, nearly the same name, 900 lines apart, and the new one's comment says "the same constant the write side uses". Keep one.
  • intensityCustom: 1 carrying a real weight returns bodyweight: True and drops the weight (:997). Unobserved case; by this PR's own principle a load that contradicts its marker is unknown, not bodyweight — the *_raw pair is the safer report.
  • No human-readable exercise name. Strength steps return name: "T1061" and overview: "sid_strength_squats". The consumer is an LLM, which will read "T1061" back to the athlete. _readable_overview() (:1694) already turns that into "Squats" — worth adding as a separate field, keeping the raw overview for the write side.
  • intensity_low / intensity_high survive on group headers (0 / None noise) — they're dropped for strength for exactly this reason, and a header has no intensity either.
  • _parse_exercise:857-859 handles None twice: ex.get("sportType", program_sport) already falls back, then if sport is None repeats it. Only the explicit-null case needs the second line — worth a comment or a merge into one expression.

Verdict

Requesting changes on (1), a decision from you on (2), and the doc catch-up in (3). The nits are optional. Substance and evidence are solid — with those three addressed this should go in.

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.

list_workout_templates reports strength reps as duration_seconds

2 participants