Add duration_open: manual/lap-press steps for workouts

[fabxyz]

Summary

targetType=1 was assumed broken — a comment on _target_fields() said it "silently produces a zero-duration, zero-distance step with no error" and left it unused. That zero is actually the wire encoding for an open/manual step: no clock or distance cap, the step only ends when the athlete presses lap on the watch.

Confirmed by building a manual-duration step in the Coros app itself (a workout named flexible_lap_press) and reading back its raw exercises via /training/program/query — open steps there carry targetType=1, targetValue=0, the same convention as the existing duration_minutes/duration_meters encodings. Field-tested on real hardware: the watch shows no countdown and waits for a lap press before advancing to the next step.

• coros_api.py: _target_fields() gains a third mutually-exclusive duration key, duration_open, alongside duration_minutes and duration_meters. Updated the stale comment and validation error messages accordingly.
• server.py: save_workout_template docstring documents duration_open (schedule_workout shares the same step-shape docs by reference).
• CLAUDE.md: one small factual addition — region can mismatch geography and cause 1019s even after a login that returns a valid-looking token. Confirmed live: an account based in Germany needed us region, not eu.
• Tests: renamed the two validation tests to match their new error messages (three duration keys now, not two), and added coverage for duration_open alone, inside a repeat group, and its (lack of) contribution to estimated_time.

Useful for warm-up/cool-down/inter-block rest whose real length varies in practice (group pace, a break, starting early) and shouldn't be forced onto a clock.

Test plan

• pytest — 228 passed, 0 failed
• ruff check — clean
• Field-tested on a real Coros watch: an open step shows no countdown and correctly waits for a lap press before advancing

🤖 Generated with Claude Code

[cygnusb]

Review

Lokal auf ed70b40 nachgeprüft: 228 Tests grün, ruff check sauber — die Angaben im Test-Plan stimmen.

Die Kernidee ist gut belegt (App-Capture + Read-Back über /training/program/query + Hardware-Test), das Feature ist rückwärtskompatibel, und die Verallgemeinerung der Exklusivitätsprüfung auf drei Keys ist sauber gelöst. Fünf Punkte vor dem Merge — die ersten drei sind substanziell.

1. Read-Path kennt targetType=1 nicht (konkreter Bug)

_parse_workout() (coros_mcp/coros_api.py:729) verzweigt nur auf targetType == 5; alles andere landet im else-Zweig als duration_seconds = targetValue. Nachgestellt:

_parse_workout({... 'exercises': [{'targetType': 1, 'targetValue': 0, ...}]})
# → {'name': 'Warm-up', 'intensity_low': 120, ..., 'duration_seconds': 0}

Ein Open-Step, den save_workout_template schreibt, kommt über list_workout_templates also als 0-Sekunden-Zeitschritt zurück — nicht unterscheidbar von einem echten Null-Step. Ein Agent, der ein Template liest und neu schreibt, verliert die Open-Semantik still (oder „repariert" den vermeintlichen 0-Minuten-Schritt). Der Write-Path lernt hier ein Encoding, das der Read-Path nicht zurückübersetzt.

Fix analog zum Distanz-Zweig:

if ex.get("targetType") == 1:
    parsed["duration_open"] = True
elif ex.get("targetType") == 5:
    ...

2. Repeat-Group mit Open-Sub-Steps ist wire-seitig unverifiziert

Bei einer Gruppe, deren Sub-Steps alle offen sind, wird iteration_seconds = 0 — der Header geht als targetType=2, targetValue=0 raus. Das ist exakt die Form, die laut Kommentar in coros_api.py:986 ff. der Server bei reinen Distanzgruppen zu einem Distanz-Header (targetType=5, Meter ×100) normalisiert. Was er mit einer Gruppe macht, deren Iteration weder Zeit noch Distanz hat, ist offen. Nachgestellt:

all-open repeat group      → header (2, 0, isGroup) + (1, 0)
distance+open repeat group → header (2, 0, isGroup) + (5, 40000) + (1, 0)

Der Hardware-Test deckt laut Beschreibung einen einfachen Open-Step ab; test_open_step_in_repeat_group ist reiner Payload-Unit-Test. Da Coros bei falschen Encodings 0000 zurückgibt und still Nullen liefert, würde ich die zwei Shapes oben einmal live durchschieben (schedule → fetch_schedule_raw → remove_scheduled_workout mit fernem HAPPEN_DAY), bevor die Doku Repeat-Gruppen mit Open-Steps empfiehlt — der neue Test-Docstring tut das mit dem „jog back down the hill"-Beispiel bereits.

3. _summarize_steps / Mixed-Warnung wurden nicht mitgezogen

server.py:175–225 kennt weiterhin nur duration_minutes und duration_meters. Folge (nachgestellt):

[open Warm-up, 10min Tempo]  → total_minutes=10.0, distance=0.0, mixed_warning=False
[nur open]                   → total_minutes=0.0,  distance=0.0, mixed_warning=False

save_workout_template gibt total_minutes: 10.0 ohne jeden Vorbehalt zurück — der Agent meldet dem Nutzer „10-Minuten-Workout". Die _MIXED_DURATION_WARNING-Mechanik wurde genau für diese Klasse von Untertreibung gebaut (Docstring: „each contributes 0 to the total it doesn't measure rather than a guess … see the mixed-duration warning"), greift hier aber nicht. Entweder Open-Steps in _has_mixed_durations() einbeziehen oder einen eigenen Hinweis ergänzen („enthält N Schritte ohne feste Länge").

4. duration_open: False — Präsenz vs. Truthiness inkonsistent

Die Exklusivitätsprüfung geht über k in s, der Branch über s.get("duration_open"). Ergebnis:

{"duration_minutes": 5, "duration_open": False}
  → ValueError: must set exactly one of ... got ['duration_minutes', 'duration_open']
{"duration_open": False}
  → ValueError: needs duration_minutes, duration_meters, or duration_open   # nennt einen Key, der da ist

Ein LLM-Client, der duration_open: false explizit setzt (naheliegend bei einem bool-Feld), bekommt einen harten Fehler für ein wohlgeformtes Zeit-Step. Entweder nur truthy Keys in duration_keys aufnehmen, oder falsy duration_open explizit mit eigener Meldung ablehnen („duration_open must be True; omit the key otherwise").

5. Doku unvollständig

Zwei Stellen sagen weiterhin „zwei Keys":

• coros_mcp/coros_api.py:1146 — Docstring von save_workout_template (API-Ebene): „(exactly one of the two)"
• README.md:348 — „Exactly one of the two keys per step." Das ist die Anwender-Doku; duration_open taucht dort gar nicht auf.

Kleinigkeiten

• CLAUDE.md-Absatz zur Region: sachlich plausibel und nützlich, aber thematisch nicht Teil dieses PRs und mit vier Sätzen recht lang für eine Datei, die in jeden Context geladen wird — ein bis zwei Sätze würden reichen. Wirklich helfen würde der Hinweis dort, wo der 1019 auftritt: in der Fehlermeldung des Auth-Pfads.
• Tests: gute Abdeckung der drei relevanten Achsen (allein, in der Gruppe, estimated_time), und die Docstrings dokumentieren die Evidenz — passt zum Stil des Repos.

Fazit: Punkt 1 (Read-Path) und Punkt 3 (falsche Zahlen an den Nutzer) würde ich als Merge-Blocker sehen. Punkt 2 ist eine Verifikationsfrage: entweder live prüfen oder den Repeat-Group-Fall vorerst nicht in der Doku bewerben.

[cygnusb]

Verified locally on ed70b40: 228 tests pass, ruff check clean — the test plan checks out.

The core finding is well evidenced (app capture + read-back via /training/program/query + hardware test), the feature is backward compatible, and generalizing the mutual-exclusion check to three keys is done cleanly. Five points before merge — the first three are substantive.

1. The read path doesn't know targetType=1 (concrete bug)

_parse_workout() (coros_mcp/coros_api.py:729) only branches on targetType == 5; everything else falls into the else arm as duration_seconds = targetValue. Reproduced:

_parse_workout({... 'exercises': [{'targetType': 1, 'targetValue': 0, ...}]})
# → {'name': 'Warm-up', 'intensity_low': 120, ..., 'duration_seconds': 0}

So an open step written by save_workout_template reads back through list_workout_templates as a zero-second time step, indistinguishable from a genuinely empty one. An agent that reads a template and writes it back silently loses the open semantics (or "fixes" the apparent 0-minute step). The write path now speaks an encoding the read path can't translate back.

Fix, mirroring the distance arm:

if ex.get("targetType") == 1:
    parsed["duration_open"] = True
elif ex.get("targetType") == 5:
    ...

2. Repeat groups with open sub-steps are unverified on the wire

For a group whose sub-steps are all open, iteration_seconds = 0, so the header goes out as targetType=2, targetValue=0. That is exactly the shape the comment at coros_api.py:986 ff. says the server normalizes into a distance header (targetType=5, meters ×100) for pure-distance groups. What it does with a group whose iteration has neither time nor distance is unknown. Reproduced:

all-open repeat group      → header (2, 0, isGroup) + (1, 0)
distance+open repeat group → header (2, 0, isGroup) + (5, 40000) + (1, 0)

The hardware test, per the description, covers a plain open step; test_open_step_in_repeat_group is a payload-shape unit test only. Since Coros returns 0000 for wrong encodings and silently yields zeros, I'd round-trip both shapes above against the live API (schedule → fetch_schedule_raw → remove_scheduled_workout, far-future HAPPEN_DAY) before the docs recommend open steps inside repeat groups — which the new test docstring already does with its "jog back down the hill" example.

3. _summarize_steps / the mixed-duration warning weren't updated

server.py:175–225 still only knows duration_minutes and duration_meters. Consequence (reproduced):

[open warm-up, 10min tempo]  → total_minutes=10.0, distance=0.0, mixed_warning=False
[open only]                  → total_minutes=0.0,  distance=0.0, mixed_warning=False

save_workout_template returns total_minutes: 10.0 with no caveat at all, so the agent reports a "10-minute workout" to the user. The _MIXED_DURATION_WARNING machinery exists for exactly this class of understatement (docstring: "each contributes 0 to the total it doesn't measure rather than a guess … see the mixed-duration warning"), but doesn't fire here. Either include open steps in _has_mixed_durations(), or add a separate note ("contains N steps with no fixed length").

4. duration_open: False — presence vs. truthiness are inconsistent

The exclusivity check uses k in s, the branch uses s.get("duration_open"). Result:

{"duration_minutes": 5, "duration_open": False}
  → ValueError: must set exactly one of ... got ['duration_minutes', 'duration_open']
{"duration_open": False}
  → ValueError: needs duration_minutes, duration_meters, or duration_open   # names a key that is already there

An LLM client that explicitly sets duration_open: false — a natural thing to do with a boolean field — gets a hard error for a well-formed time step. Either only collect truthy keys into duration_keys, or reject a falsy duration_open with its own message ("duration_open must be True; omit the key otherwise").

5. Docs left incomplete

Two places still say "two keys":

• coros_mcp/coros_api.py:1146 — save_workout_template docstring (API level): "(exactly one of the two)"
• README.md:348 — "Exactly one of the two keys per step." That's the user-facing doc, and duration_open doesn't appear there at all.

Minor

• The CLAUDE.md region paragraph: factually plausible and useful, but off-topic for this PR and rather long (four sentences) for a file loaded into every context — one or two would do. Where it would really help is at the point the 1019 surfaces: in the auth path's error message.
• Tests: good coverage of the three relevant axes (standalone, inside a group, estimated_time), and the docstrings record the evidence — matches the style of the repo.

Summary: I'd treat point 1 (read path) and point 3 (wrong numbers reported to the user) as merge blockers. Point 2 is a verification question: either check it live, or hold off on advertising the repeat-group case in the docs.

[fabxyz]

Pushed f927b6a — all five points addressed. 239 passed, ruff clean, mypy clean.

1. _parse_workout learns targetType=1 → duration_open: True, with a write→read round-trip test.
2. _OPEN_DURATION_WARNING + _count_open_steps (counts each repetition inside a repeat group); both tools now go through _attach_duration_warnings, which concatenates with the mixed-unit warning. _attach_mixed_duration_warning and its tests unchanged.
3. Ignored a falsy duration_open rather than rejecting it — rejecting still hard-errors on {"duration_minutes": 5, "duration_open": false}, which is well-formed. The "no duration key" error now says so explicitly when a falsy duration_open is present.
4. API docstring + README, plus the module-level targetType legend and both tools' Returns sections.

On 2 — agreed, not verified live. The builder now documents that the unverified shape is specifically a group whose iteration has neither time nor distance (all-open, or open+distance); a group mixing open with a timed sub-step still carries the timed seconds in its header, which is what test_open_step_in_repeat_group covers. Kept out of the README until I can run schedule → read-back → remove on hardware.

Unrelated pre-existing find while in _parse_workout, filed separately as #60.

[cygnusb]

Verified on f927b6a: 239 tests pass, ruff check clean, mypy clean (13 files).

I re-ran each of the five points against the new head rather than reading the diff alone:

#	before	now
1	targetType=1 → duration_seconds: 0	→ {'duration_open': True}, no duration_seconds ✔
3	[open, 10min] → bare total_minutes=10.0	→ warning "has 1 open step(s) … The real workout is longer" ✔
4	{"duration_minutes": 5, "duration_open": False} → ValueError	→ accepted, targetType=2 ✔
4b	falsy-only duration_open → misleading message	→ "duration_open is present but falsy — it must be True" ✔
5	docstring + README said "two"	both updated, plus the module-level targetType legend ✔

Two of the fixes are better than what I suggested:

• Point 3: a separate _OPEN_DURATION_WARNING + _attach_duration_warnings collector, instead of widening _has_mixed_durations. The existing path and its tests stay untouched and the two caveats concatenate cleanly — right call, they have different causes.
• Point 4: the truthiness exception is scoped to duration_open only (k in s and (k != "duration_open" or s[k])), so duration_meters: 0 still correctly fails with "must be positive".

The region hint moved to _check_response is safe with the retry path: _run_with_auth matches _AUTH_RESULT_CODES against the result code, not the message text.

Point 2 — agreed as a documented deferral. The comment at coros_api.py:1014 ff. draws the line precisely (an iteration with neither time nor distance is the unverified shape; open + timed is not), and it isn't advertised in the README. Good until the live round-trip happens.

I pushed one follow-up commit to this branch (263814e): the doc sweep covered the write-side tools but not list_workout_templates, which is the tool that actually emits the new key — its Returns section still promised "either duration_seconds … or distance_meters". README's entry for it named only duration_seconds (it never mentioned distance_meters either); both are listed now. Tests/ruff/mypy still clean.

Two notes, neither needing action:

• _count_open_steps() counts executions (16 for a 16× group) while steps_count counts structure (2). Both land in the same response dict, so an agent reads steps_count: 2 next to "has 16 open step(s)". The choice is deliberate and documented, and it's the more useful number for the warning — only the wording ("16 open step executions") would remove the friction.
• _REGION_HINT is appended to every 1019, including the expired-token ones _run_with_auth retries; if that retry fails for another reason the region advice reads as a red herring. The "if this persists right after a successful login" hedge covers it.

Nice work on the evidence trail — the test docstrings recording how each encoding was confirmed are what makes this reviewable at all. Answering #60's three questions separately.