Repository navigation
list_workout_templates reports strength reps as duration_seconds #60
Description
Activity
Good catch, and thanks for asking first. Reproduced — but the diagnosis is incomplete, and that changes the answer to question 1:
_parse_workout({'sportType': 4, 'exercises': [ {'name':'Squat', 'targetType':3, 'targetValue':12, 'sets':3, 'intensityValue':27900, 'intensityCustom':0}, {'name':'Push-up', 'targetType':3, 'targetValue':15, 'sets':3, 'intensityValue':'', 'intensityCustom':1}]}) # → [{'name':'Squat', ..., 'intensity_low': 27900, 'duration_seconds': 12}, # {'name':'Push-up', ..., 'intensity_low': '', 'duration_seconds': 15}]
It isn't only the reps that are mislabelled — the weight is broken too:
27900instead of27.9 kg(the write side encodes kg×1000, or lbs inintensityPercentwithintensityDisplayUnit6/7), and bodyweight comes back as an empty string becauseintensityCustom=1is the iOS marker for it. The strength namespace diverges on three axes, not one.1. One parser or two? → Two
A reps branch alone would leave
list_workout_templatesstill wrong for strength (the weight). And each additional axis you branch on inside_parse_workoutturns a function with a clear contract ("endurance step → step dict") into one carrying two vocabularies the reader has to keep apart. Concretely:_parse_workout()stays endurance-only, contract unchanged (your existing tests stay valid).- new
_parse_strength_workout()with the weight decoding. - dispatch one level up in
fetch_workout_templates()onsportType == 4— that's the only caller of_parse_workoutin the repo, so the change stays local. - put the shared header fields (
id,name,sport_type,sport_name,estimated_time_seconds,exercise_count) in a small_parse_workout_header()helper; the duplication you're worried about is exactly those six lines.
2. Key names
- Reps: plain
reps. Noduration_*prefix — it isn't a duration. targetType=2staysduration_seconds. Seconds are seconds in both namespaces; don't rename something that's correct.- Weight:
weight_kg(fromintensityValue/1000) andbodyweight: Truefor the''/intensityCustom=1marker — i.e. the same names the write side takes as input. That's the principle Add duration_open: manual/lap-press steps for workouts #59 established withduration_open: read output and write input speak the same vocabulary, otherwise read-modify-write is impossible. - Strength steps should then carry no
intensity_low/intensity_highat all — those fields mean nothing there, and omitting them is more honest than surfacing a raw wire value.
3. Downstream
Nothing.
_parse_workouthas exactly one caller (fetch_workout_templates→ thelist_workout_templatestool), no internal path readsduration_secondsonward, and no test depends on strength values (the three hits undertests/are endurance cases). The only consumer is the LLM on the other end, which is better served by correct keys than by the stability of a wrong number. It's an add rather than a rename anyway:duration_secondsonly disappears where it never meant seconds.The bigger point
#59 and #60 are the same bug twice, and the cause is one line:
else: parsed["duration_seconds"] = ex.get("targetValue")
An
elsethat reports every unknowntargetTypeas seconds is guaranteed to produce another one of these the moment Coros adds a fourth type (calories, HR target, …). While you're in there, please make the fallback honest: known type → named key, unknown type →target_type_raw/target_value_rawinstead of a guessed unit. Then the next unknown type is visible rather than silently wrong, and a read-modify-write can at least pass it through untouched.Worth capturing the wire-encoding table (targetType per namespace) in one place in the code while you're at it, rather than spread across comments on the read and write sides.
Please go ahead and implement it along those lines — you have the context. Two asks: keep the same evidence discipline as #59 (a real strength template read back from the library as the reference for the weight decoding, recorded in the test docstrings), and treat the endurance parser's existing tests as the regression fence — they shouldn't need to change.
Agreed on all of it — two parsers, dispatch in
fetch_workout_templates,reps/duration_seconds/weight_kg+bodyweight, and the honest fallback. The fallback is the part I'm happiest about: it's the only bit that stops this recurring.Starting on it. Three things your spec doesn't cover that expand the output shape, so flagging the defaults I'll implement rather than guessing silently:
1. Pounds.
weight_kgfromintensityValue/1000is kg-only, but the write side stores lbs as both a kg-equivalent inintensityValueandintensityPercent = lbs × 1e6, withintensityDisplayUnit: "7"(all four cases tested intest_workout_payloads.py). Decoding kg-only round-trips an lbs exercise into kg — numerically equivalent, but the athlete's display unit silently flips. Planning to dispatch onintensityDisplayUnit:"6"→weight_kg,"7"→weight_lbsfromintensityPercent.2.
0 kgis not bodyweight.''+intensityCustom=1→bodyweight: True;weight_kg=0+intensityCustom=0→ a literal "0.00 kg" the write side deliberately keeps distinct. I'll key offintensityCustom, not falsiness.3. Round-trip needs more than reps + weight. By the vocabulary principle: rebuilding a strength template also needs
origin_id(the write side requires it — without it a read template can't be rewritten at all),overview(thesid_key),rest_seconds(restType=3→ 0,restType=1→restValue), and per-exercisesets. Alsosets/totalSetsat program level for circuit rounds, which the six-field header helper doesn't carry. Planning to include all of these — say if you'd rather keep the strength output lean and treat full round-trip as a separate step.Evidence first, as in #59: nothing in the repo has a strength template read back, so whether the list endpoint even returns
intensityCustom/intensityDisplayUnit/intensityPercent/originIdper exercise is still unverified (_EXERCISE_DROPstripping some of them in the raw-program view hints they're there, but that's a different endpoint). I'll capture a kg, a bodyweight and an lbs template from a real library read and record them in the test docstrings before writing the decoder.Reviewed your three flags against the code. All three hold — go ahead with the defaults you proposed. Answers below, plus six things neither of us has covered yet, and one place where my own vocabulary argument doesn't survive contact with the write side.
Your three, decided
1. Pounds → yes, dispatch on
intensityDisplayUnit. Confirmed atcoros_api.py:1522-1546and pinned bytest_workout_payloads.py:77-88. Decoding kg-only would flip the athlete's display unit on a read-modify-write;"6"→weight_kg,"7"→weight_lbsfromintensityPercent/1e6is right.2.
0 kgis not bodyweight → yes, key offintensityCustom.coros_api.py:1526-1528keeps the two deliberately distinct on the write side; falsiness would collapse them.3. Round-trip fields → include them, with one subtraction.
origin_idis not optional —ex["origin_id"]is a hardKeyErroratcoros_api.py:1567, so a strength template that can't be rewritten at all is the status quo you're fixing.overview,rest_secondsand per-exercisesetslikewise.But not
totalSets: the strength builder sets program-levelsetsandtotalSetsto the same circuit count (coros_api.py:1628/1632), while the endurance builder setstotalSets = real_step_count(:1158). The field means different things per namespace. Readsetsonly, and keep it out of the shared_parse_workout_header()— it belongs to the strength parser.One correction to the round-trip framing while you're there: re-scheduling an unmodified template already works today and always did.
schedule_workout_templategoes through_fetch_raw_workout(coros_api.py:1803) and posts the raw library item back inline — it never touches_parse_workout. What the new fields unlock is specifically read → modify →save_strength_workout_template. Worth saying plainly in the PR description so the change isn't credited with fixing something that isn't broken.Where my own argument in the last comment was too clean
I justified
weight_kg/bodyweightwith "read output and write input speak the same vocabulary". That holds for the weight axis and breaks on the axis this issue is actually about.save_strength_workout_templatetakes (server.py:1538-1541):target_type (int): 2=time in seconds, 3=reps target_value (int)No
reps, noduration_seconds, nobodyweight. Sorepson the read side is a better name, not a matching one — andbodyweight: Truefed back into the write tool only works by accident, because the builder ignores unknown keys and reads "no weight set" as bodyweight. (Endurance is already off in the same way: read emitsduration_seconds, write takesduration_minutes.)Two ways out, and I'd rather decide it here than let it drift:
- Preferred: in this same PR, have
save_strength_workout_templateandschedule_strength_workoutalso acceptreps/duration_seconds/bodyweight, keepingtarget_type/target_valueas accepted aliases. Then the principle is true instead of aspirational, and the read output is directly pasteable into the write tool. - If that makes the PR too wide, split it — but then the read PR should not claim the round-trip property, and the follow-up issue gets filed in the same breath.
Your call which; say which one you're taking in the PR description.
Six things neither of us has raised
a. Dispatch per exercise, not per program. Both builders emit
hybridTotalSets(coros_api.py:1144,1619), so the wire format admits mixed programs. A dispatch on program-levelsportType == 4mislabels the strength steps of a hybrid template — this exact bug again, one sport further along. The write side stampssportType: 4on every exercise (:1605); dispatch per exercise with the programsportTypeas fallback costs nothing. Whether the list response carries it per exercise is one more thing for the capture below to answer.b. Wire types are not what the write side sends.
intensityDisplayUnitgoes out as a string ("6"/"7") — acceptstrandinton read.intensityValuecan come back as a string too, and_unscale(coros_api.py:723) raisesTypeErroron one. Coerce before dividing.c. lbs with a missing
intensityPercent(server normalization is a real possibility here — see #52/#55) should fall back toweight_kg, not assertweight_lbs: 0.d. The honest fallback should cover
restTypetoo.restType 3 → 0,1 → restValue, everything else → ? That is the same guessingelsethis issue exists to remove. UnknownrestType→rest_type_raw/rest_value_raw.e.
_EXERCISE_DROPcontradicts the new read path.coros_api.py:1342-1349discardsoriginId,intensityCustom,intensityDisplayUnit,isIntensityPercentfrom the raw view as noise. Those become the semantic core of the strength parser. KeeporiginIdat minimum, or leave a comment saying why the two views disagree.f. The wire table has nowhere to live yet. The endurance table is in the docstring of
_target_fields— a nested function inside_build_workout_program_payload(coros_api.py:876), unreachable from the read side. "One place" concretely means a module-level block with both columns, referenced from_target_fields,_parse_workoutand_parse_strength_workout.Also:
server.py:810-813currently promises "exactly one duration key:duration_seconds/distance_meters/duration_open". That contract changes — update it with the code.On evidence
Cheaper than you're assuming:
delete_workout_templateexists (coros_api.py:1211). So create a kg, an lbs and a bodyweight template throughsave_strength_workout_template, read them back via_fetch_raw_workout, delete them — a controlled sent→stored diff with no library residue.Do that and read back one template built in the iOS app. The server rewrites payloads (confirmed in #52/#55, where a group header came back re-encoded), so what our builder sends is not automatically what the app sends, and the app's version is the ground truth for the read format. Record both in the test docstrings as in #59.
Please open the PR
Nothing here needs another round of sign-off — the three defaults are approved as you proposed them (minus
totalSets), and the vocabulary question above is the only real choice left, which you can make in the PR description rather than in a comment. Open it againstmainas a draft if the live capture isn't done yet; the endurance parser's existing tests stay untouched as the regression fence.- Preferred: in this same PR, have
Found while working on #59 — pre-existing and unrelated to that PR. Asking before I write anything, because the fix has a design decision in it that's yours to make.
What's wrong
list_workout_templatesreports the reps of a strength exercise as if they were seconds. A squat saved as 3 sets × 12 reps reads back as:{"name": "Squat", "sets": 3, "duration_seconds": 12}So an agent listing the library tells the user "3 sets of 12 seconds". The number survives, the unit is wrong, and nothing in the response signals it — the value looks perfectly plausible as a duration.
Why
Coros encodes every step as a
targetTypetag plus a baretargetValue, and the tag's vocabulary differs per namespace:targetType_parse_workoutonly knows the endurance column:targetType == 5→ distance, (as of #59)targetType == 1→ open, everything else →duration_seconds. That "everything else" swallows strength'stargetType=3, which_build_strength_program_payloaddocuments on the write side astarget_type: 2=time (seconds), 3=reps.It surfaces because
fetch_workout_templatesdoesn't filter by sport — it lists every template in the library, strength included.Same shape as the read-path gap you caught in #59 — the write side knows an encoding the read side doesn't — just in the strength namespace, and much older than that PR.
Questions before I touch it
_parse_workoutnever inspectssportTypetoday. Making it sport-aware changes its contract; giving strength its own parser keeps each one honest but duplicates the shared fields. Which do you prefer?targetType == 2stayduration_secondsfor strength, or get a strength-flavoured name alongside reps?duration_secondsoff strength templates that a rename would break?