From ac99c8b97ead33576586cfe5e0e05245b7dc573a Mon Sep 17 00:00:00 2001 From: friendsa Date: Sat, 1 Aug 2026 13:27:00 +0000 Subject: [PATCH 1/2] SHA-21: FILE_TYPE validation for Workout files Add Workout required messages (workout + workout_step) and fields under ConformanceLevel.FILE_TYPE. Activity rules unchanged; Course and other unimplemented file_id.type values still fail closed. SDK Workout fixtures and strict builder paths are covered; README, design doc status, and Towncrier fragment updated. Co-authored-by: multica-agent --- README.md | 6 +- docs/FIT_CONFORMANCE_DESIGN.md | 23 ++++---- fit_tool/tests/test_validation.py | 82 +++++++++++++++++++++++++++- fit_tool/tests/test_workout_files.py | 74 +++++++++++++++++++++++++ fit_tool/validation.py | 60 ++++++++++++++++++-- news/SHA-21.feature | 5 ++ 6 files changed, 228 insertions(+), 22 deletions(-) create mode 100644 news/SHA-21.feature diff --git a/README.md b/README.md index 4f18788..2cd8269 100644 --- a/README.md +++ b/README.md @@ -24,8 +24,8 @@ package already implements. Profile version in use: `21.205.0` (see | Status | Capabilities | | --- | --- | | **Supported** | Common Activity and Workout read/write via typed profile messages; `FitFileBuilder` encode path; **header CRC** (14-byte headers) and **file-level CRC** on load / stream exhaustion (`check_crc=True` default); developer fields for common declaration patterns; streaming iterators (`FitFile.iter_file` / `iter_stream`); CSV export (`to_csv` / `to_rows`); Course and similar message types when you construct them yourself; **chained multi-segment** FIT decode via `from_bytes` / `from_file` (all segments projected into `records`); **compressed timestamp** reconstruction into field 253; **subfield resolution** (ref-field match → type / scale / offset / units; multi-ref AND; first match wins); **component expansion** for all Profile main-field sources (generated registry) **and** components on the active subfield; nested expansion + accumulator rollover; **unknown field ids** on known messages as `UnknownField` (decoded values + `raw_bytes`); **encode modes** `EncodeMode.PRESERVE` (default; unedited bit-identical + post-edit dirty re-project) and `EncodeMode.CANONICAL` (full re-project, normalized sizes/CRCs; optional `strict=True` precheck) — see [Encode policies](#encode-policies) | -| **Partial** | Unknown global messages via `GenericMessage` (readable; unedited preserve keeps wire bytes; post-edit re-encodes dirty records only); composable validation API (`validate_fit_file` / `FitFile.validate`) with WIRE + PROFILE + Activity FILE_TYPE levels — **PROFILE validation is CORE today** (developer-field subset + **ambiguous subfield** ERROR); opt-in **PRESERVATION** level reports unknown-field `raw_bytes` loss after edits; architecture decision **O1** keeps bundled `Profile.xlsx` as the full metadata source of truth with future DOMAIN/FULL scopes (FULL opt-in, not default strict) — see [`docs/FIT_CONFORMANCE_DESIGN.md`](docs/FIT_CONFORMANCE_DESIGN.md) §3.1; Builder `strict=True` is a thin wrapper over the same checks (WIRE+PROFILE+FILE_TYPE only) | -| **Not supported / incomplete** | Full PROFILE semantics (native field requirements, enums, units beyond subfield scale/units); file-type rules for non-Activity types (FILE_TYPE level fails closed); intentional `repair()` API (strict path never silent-repairs); bit-identical rewrite of compressed-timestamp dirty records when field 253 is not on the definition (strict raises; non-strict keeps compressed header) | +| **Partial** | Unknown global messages via `GenericMessage` (readable; unedited preserve keeps wire bytes; post-edit re-encodes dirty records only); composable validation API (`validate_fit_file` / `FitFile.validate`) with WIRE + PROFILE + **Activity and Workout** FILE_TYPE levels — **PROFILE validation is CORE today** (developer-field subset + **ambiguous subfield** ERROR); opt-in **PRESERVATION** level reports unknown-field `raw_bytes` loss after edits; architecture decision **O1** keeps bundled `Profile.xlsx` as the full metadata source of truth with future DOMAIN/FULL scopes (FULL opt-in, not default strict) — see [`docs/FIT_CONFORMANCE_DESIGN.md`](docs/FIT_CONFORMANCE_DESIGN.md) §3.1; Builder `strict=True` is a thin wrapper over the same checks (WIRE+PROFILE+FILE_TYPE only) | +| **Not supported / incomplete** | Full PROFILE semantics (native field requirements, enums, units beyond subfield scale/units); file-type rules for types other than Activity/Workout (e.g. Course — FILE_TYPE fails closed); intentional `repair()` API (strict path never silent-repairs); bit-identical rewrite of compressed-timestamp dirty records when field 253 is not on the definition (strict raises; non-strict keeps compressed header) | If you need a construct listed as incomplete, prefer an official Garmin SDK or wait for the phased work in the design doc (including the **Remaining gaps** @@ -221,7 +221,7 @@ Validation is a first-class API. Levels match the design doc | --- | --- | --- | | `ConformanceLevel.WIRE` | Local IDs, definition field layout/sizes, data records vs active definition | Implemented | | `ConformanceLevel.PROFILE` | Developer field declarations (`developer_data_id` / `field_description`) and base-type consistency; **ambiguous native subfields** (more than one Profile match) as ERROR | **CORE scope today** (+ ambiguous-subfield ERROR). Roadmap: DOMAIN then FULL rules from Profile.xlsx; FULL is opt-in, not default `strict` — design doc §3.1 (O1). Subfield *resolution* for decode/encode is separate and supported | -| `ConformanceLevel.FILE_TYPE` | `file_id` first/unique + required fields; Activity required messages and fields | **Activity only**; other `file_id.type` values **fail closed** (intentional until more validators exist) | +| `ConformanceLevel.FILE_TYPE` | `file_id` first/unique + required fields; Activity and Workout required messages and fields | **Activity + Workout**; other `file_id.type` values (e.g. Course) **fail closed** (intentional until more validators exist) | | `ConformanceLevel.PRESERVATION` | Post-edit rewrite loss (e.g. `UnknownField.raw_bytes` cleared by mutation) | **Opt-in** — not in default levels / Builder `strict=True` | Call validation on any `FitFile` or record list — after decode or before encode: diff --git a/docs/FIT_CONFORMANCE_DESIGN.md b/docs/FIT_CONFORMANCE_DESIGN.md index 1e35f57..786df86 100644 --- a/docs/FIT_CONFORMANCE_DESIGN.md +++ b/docs/FIT_CONFORMANCE_DESIGN.md @@ -22,8 +22,8 @@ Baseline after wire-layer work (#44 / #45 and follow-ups on `main`): | **Ambiguous subfields** (more than one match) | PROFILE ERROR (decode still uses first match) | Same policy | | **Preservation encode** (`to_bytes(mode=EncodeMode.PRESERVE)` / `preserve=True`, default) for buffer-decoded files: unedited path bit-identical; **post-edit** path re-encodes dirty records and copies `source_bytes` for the rest | Supported | Aligns with design §6.1 | | **Canonical encode** (`to_bytes(mode=EncodeMode.CANONICAL)` / `preserve=False`); optional `strict=True` precheck | Supported | Compressed-header expansion only when field 253 is on the definition; otherwise keep compressed (non-strict) or raise (strict) | -| Unknown global messages (`GenericMessage`); composable validation (`validate_fit_file` / `FitFile.validate`) with WIRE + PROFILE (developer fields + ambiguous-subfield ERROR) + Activity FILE_TYPE; opt-in **PRESERVATION** level; Builder `strict=True` wraps default levels only | Partial | Full Profile field/enum/units rules; Workout/Course FILE_TYPE | -| Full PROFILE semantics (native required fields, enums, units beyond subfield scale/units); non-Activity FILE_TYPE rules | Not supported / incomplete | Phases 3–4 and remaining-gaps table below | +| Unknown global messages (`GenericMessage`); composable validation (`validate_fit_file` / `FitFile.validate`) with WIRE + PROFILE (developer fields + ambiguous-subfield ERROR) + Activity/Workout FILE_TYPE; opt-in **PRESERVATION** level; Builder `strict=True` wraps default levels only | Partial | Full Profile field/enum/units rules; Course FILE_TYPE | +| Full PROFILE semantics (native required fields, enums, units beyond subfield scale/units); Course and other non-Activity/non-Workout FILE_TYPE rules | Not supported / incomplete | Phases 3–4 and remaining-gaps table below | | Unknown field ids on known messages (`UnknownField` + `raw_bytes` on decode; survive post-edit when not mutated) | Supported | Mutating an unknown field clears `raw_bytes` (PRESERVATION ERROR if that level is selected) | Until §11 Definition of Done is met, do not describe the library as fully @@ -46,8 +46,8 @@ stable keys, later letters are stage placeholders until children are created. | Post-edit PRESERVATION (edited files, dirty records) | Phase 4 / PRESERVATION level | **F** SHA-18 | **Done**: per-record dirty + mixed encode; opt-in PRESERVATION findings | | Encode policies (canonical vs preserve, strict vs repair) | Phase 4 §6 | **G** SHA-19 | **Done**: `EncodeMode` + policy matrix; no silent invalid clamp; `repair()` API still future | | Full PROFILE validation from bundled Profile.xlsx `21.205.0` | Phase 3 PROFILE | **H** (SHA-12 stage 4; child TBD) | Slice by message family / rule kind | -| Workout FILE_TYPE rules | Phase 4 §7 | **I** (SHA-12 stage 4; child TBD) | Not in first batch | -| Course FILE_TYPE rules | Phase 4 §7 | **J** (SHA-12 stage 4; child TBD) | Not in first batch | +| Workout FILE_TYPE rules | Phase 4 §7 | **I** SHA-21 | **Done**: required messages/fields for Workout; SDK Workout*.fit fixtures pass FILE_TYPE | +| Course FILE_TYPE rules | Phase 4 §7 | **J** (SHA-12 stage 4; child TBD) | Not in first batch; still fail-closed | | Public capability matrix / release-note pass at claim flip | Phase 5 | **L** (SHA-12 stage 5; child TBD) | Update README + this section together | Letter **A** is this status-truth pass (Multica **SHA-13**). Do not claim gaps @@ -539,7 +539,8 @@ report.raise_for_errors() `FitFileBuilder(strict=True)` is a thin wrapper over the same API (all default levels, raise on error). Wire-range and Definition Message checks still run on -every `add`. FILE_TYPE rules cover Activity and fail closed for other types. +every `add`. FILE_TYPE rules cover Activity and Workout; other `file_id.type` +values (e.g. Course) fail closed. **Already on the wire / compatibility path** (see Current status): layered decode (`fit_tool/wire`), chained multi-segment load, header + file CRC, @@ -550,9 +551,9 @@ messages, and preservation encode via `to_bytes(preserve=True)` for unedited re-project). This still does **not** complete the conformance claim. Remaining work includes -full Profile validation scopes (DOMAIN/FULL) and Workout/Course FILE_TYPE -validators — see **Remaining gaps** and Phases 3–5. Encode modes (G) are on the -compatibility path; the long-term `FitDocument` encode surface remains future. +full Profile validation scopes (DOMAIN/FULL) and Course FILE_TYPE validators — +see **Remaining gaps** and Phases 3–5. Encode modes (G) are on the compatibility +path; the long-term `FitDocument` encode surface remains future. ## 7. File-type validators @@ -736,9 +737,9 @@ Exit: all produced standard files pass the selected Garmin and repository validators without repair. **Progress (partial):** unedited and **post-edit** PRESERVE paths exist; explicit -`EncodeMode` / `strict` / policy matrix landed (G / SHA-19). Activity FILE_TYPE -(fail-closed other types) exists. Workout/Course validators remain Multica I–J. -`repair()` API is still future. +`EncodeMode` / `strict` / policy matrix landed (G / SHA-19). Activity and +Workout FILE_TYPE exist (I / SHA-21); other types (including Course / J) fail +closed. `repair()` API is still future. ### Phase 5: API migration and performance diff --git a/fit_tool/tests/test_validation.py b/fit_tool/tests/test_validation.py index b8713b6..297dbd0 100644 --- a/fit_tool/tests/test_validation.py +++ b/fit_tool/tests/test_validation.py @@ -11,8 +11,9 @@ from fit_tool.profile.messages.lap_message import LapMessage from fit_tool.profile.messages.record_message import RecordMessage from fit_tool.profile.messages.session_message import SessionMessage +from fit_tool.profile.messages.workout_message import WorkoutMessage from fit_tool.profile.messages.workout_step_message import WorkoutStepMessage -from fit_tool.profile.profile_type import FileType, Manufacturer, Sport +from fit_tool.profile.profile_type import FileType, Manufacturer, Sport, WorkoutStepDuration, WorkoutStepTarget from fit_tool.validation import ( ConformanceLevel, FitFileValidator, @@ -60,6 +61,29 @@ def add_minimal_activity_messages(builder, record_message=None): builder.add(activity) +def add_minimal_workout_messages(builder, step_message=None): + file_id = FileIdMessage() + file_id.type = FileType.WORKOUT + file_id.manufacturer = Manufacturer.DEVELOPMENT.value + file_id.product = 0 + file_id.serial_number = 1234 + file_id.time_created = 1_700_000_000_000 + builder.add(file_id) + + workout = WorkoutMessage() + workout.num_valid_steps = 1 + workout.sport = Sport.CYCLING + builder.add(workout) + + step = step_message if step_message is not None else WorkoutStepMessage() + step.message_index = 0 + step.duration_type = WorkoutStepDuration.TIME + step.duration_time = 600.0 + step.target_type = WorkoutStepTarget.OPEN + step.target_value = 0 + builder.add(step) + + class TestFitValidation(unittest.TestCase): def test_builder_rejects_local_id_outside_wire_range(self): @@ -98,9 +122,10 @@ def test_strict_activity_accepts_required_message_structure(self): self.assertGreater(len(encoded), 0) def test_strict_validation_fails_closed_for_unsupported_file_type(self): + # Course (and other non-Activity/non-Workout types) still fail closed. builder = FitFileBuilder(strict=True) file_id = FileIdMessage() - file_id.type = FileType.WORKOUT + file_id.type = FileType.COURSE file_id.manufacturer = Manufacturer.DEVELOPMENT.value file_id.product = 0 file_id.serial_number = 1234 @@ -110,6 +135,27 @@ def test_strict_validation_fails_closed_for_unsupported_file_type(self): with self.assertRaisesRegex(FitValidationError, 'not implemented'): builder.build_bytes() + def test_strict_workout_accepts_required_message_structure(self): + builder = FitFileBuilder(strict=True, auto_define=True, min_string_size=50) + add_minimal_workout_messages(builder) + + encoded = builder.build_bytes() + + self.assertGreater(len(encoded), 0) + + def test_strict_workout_requires_workout_and_steps(self): + builder = FitFileBuilder(strict=True) + file_id = FileIdMessage() + file_id.type = FileType.WORKOUT + file_id.manufacturer = Manufacturer.DEVELOPMENT.value + file_id.product = 0 + file_id.serial_number = 1234 + file_id.time_created = 1_700_000_000_000 + builder.add(file_id) + + with self.assertRaisesRegex(FitValidationError, 'workout'): + builder.build_bytes() + def test_strict_validation_rejects_undeclared_developer_field(self): developer_field = DeveloperField( developer_data_index=0, @@ -218,7 +264,7 @@ def test_validate_fit_file_raise_mode_matches_strict_builder(self): def test_validate_wire_only_skips_file_type_rules(self): builder = FitFileBuilder() file_id = FileIdMessage() - file_id.type = FileType.WORKOUT + file_id.type = FileType.COURSE file_id.manufacturer = Manufacturer.DEVELOPMENT.value file_id.product = 0 file_id.serial_number = 1234 @@ -235,6 +281,15 @@ def test_validate_wire_only_skips_file_type_rules(self): any('not implemented' in finding.message for finding in full_report.errors) ) + def test_validate_workout_file_type_report(self): + builder = FitFileBuilder(auto_define=True, min_string_size=50) + add_minimal_workout_messages(builder) + fit_file = builder.build() + + report = validate_fit_file(fit_file, levels={ConformanceLevel.FILE_TYPE}) + self.assertFalse(report.has_errors) + self.assertEqual(report.findings, []) + def test_fit_file_validator_legacy_facade(self): builder = FitFileBuilder() add_minimal_activity_messages(builder) @@ -662,6 +717,27 @@ def test_file_type_findings_structure_errors(self): self.assertTrue(report5.has_errors) self.assertTrue(any('missing required field' in f.message for f in report5.errors)) + # Workout: missing required step fields + w_builder = FitFileBuilder(auto_define=True, min_string_size=20) + w_file_id = FileIdMessage() + w_file_id.type = FileType.WORKOUT + w_file_id.manufacturer = Manufacturer.DEVELOPMENT.value + w_file_id.product = 0 + w_file_id.serial_number = 1 + w_file_id.time_created = 1_700_000_000_000 + w_builder.add(w_file_id) + w_msg = WorkoutMessage() + # num_valid_steps intentionally unset + w_builder.add(w_msg) + bare_step = WorkoutStepMessage() + bare_step.workout_step_name = 'x' + # message_index / duration_type / target_type unset + w_builder.add(bare_step) + w_fit = w_builder.build() + report6 = validate_fit_file(w_fit, levels={ConformanceLevel.FILE_TYPE}) + self.assertTrue(report6.has_errors) + self.assertTrue(any('missing required field' in f.message for f in report6.errors)) + def test_legacy_validator_helpers(self): builder = FitFileBuilder() add_minimal_activity_messages(builder) diff --git a/fit_tool/tests/test_workout_files.py b/fit_tool/tests/test_workout_files.py index 496b8fc..f1a8ca5 100644 --- a/fit_tool/tests/test_workout_files.py +++ b/fit_tool/tests/test_workout_files.py @@ -3,9 +3,25 @@ import os import sys import unittest +from pathlib import Path from fit_tool.fit_file import FitFile +from fit_tool.fit_file_builder import FitFileBuilder +from fit_tool.profile.messages.file_id_message import FileIdMessage +from fit_tool.profile.messages.workout_message import WorkoutMessage +from fit_tool.profile.messages.workout_step_message import WorkoutStepMessage +from fit_tool.profile.profile_type import ( + FileType, + Manufacturer, + Sport, + WorkoutStepDuration, + WorkoutStepTarget, +) from fit_tool.utils.logging import formatter, logger +from fit_tool.validation import ConformanceLevel, validate_fit_file + +SDK_DIR = Path(__file__).resolve().parent / 'data' / 'sdk' +WORKOUT_SDK_FILES = sorted(SDK_DIR.glob('Workout*.fit')) class TestCourseFiles(unittest.TestCase): @@ -29,3 +45,61 @@ def test_decode_trainer_road(self): fit_file = FitFile.from_bytes(bytes_buffer) print(f'Profile version: {fit_file.header.profile_version}') fit_file.to_rows() + + +class TestWorkoutFileTypeValidation(unittest.TestCase): + """FILE_TYPE rules for Workout (SHA-21 / design Phase 4 §7).""" + + def test_sdk_workout_fixtures_pass_file_type(self): + self.assertTrue(WORKOUT_SDK_FILES, 'expected committed SDK Workout*.fit fixtures') + for path in WORKOUT_SDK_FILES: + with self.subTest(path=path.name): + fit_file = FitFile.from_file(str(path)) + report = validate_fit_file(fit_file, levels={ConformanceLevel.FILE_TYPE}) + self.assertFalse( + report.has_errors, + f'{path.name} FILE_TYPE errors: {[f.message for f in report.errors]}', + ) + + def test_trainerroad_workout_passes_file_type(self): + path = Path(__file__).resolve().parent / 'data' / 'trainerroad_744490.fit' + fit_file = FitFile.from_file(str(path)) + report = validate_fit_file(fit_file, levels={ConformanceLevel.FILE_TYPE}) + self.assertFalse( + report.has_errors, + f'FILE_TYPE errors: {[f.message for f in report.errors]}', + ) + + def test_strict_builder_builds_minimal_workout(self): + builder = FitFileBuilder(strict=True, auto_define=True, min_string_size=50) + file_id = FileIdMessage() + file_id.type = FileType.WORKOUT + file_id.manufacturer = Manufacturer.DEVELOPMENT.value + file_id.product = 0 + file_id.serial_number = 0x12345678 + file_id.time_created = 1_700_000_000_000 + builder.add(file_id) + + step = WorkoutStepMessage() + step.message_index = 0 + step.workout_step_name = 'Warm up' + step.duration_type = WorkoutStepDuration.TIME + step.duration_time = 600.0 + step.target_type = WorkoutStepTarget.OPEN + step.target_value = 0 + + workout = WorkoutMessage() + workout.workout_name = 'Tempo' + workout.sport = Sport.CYCLING + workout.num_valid_steps = 1 + + builder.add(workout) + builder.add(step) + encoded = builder.build_bytes() + self.assertGreater(len(encoded), 0) + + report = validate_fit_file( + FitFile.from_bytes(encoded), + levels={ConformanceLevel.FILE_TYPE}, + ) + self.assertFalse(report.has_errors) diff --git a/fit_tool/validation.py b/fit_tool/validation.py index 77b77fa..3b8efc6 100644 --- a/fit_tool/validation.py +++ b/fit_tool/validation.py @@ -10,13 +10,14 @@ ``field_description``) plus **ambiguous native subfield** matches. This is **not** full Garmin Profile validation (enums, units, required native fields per message, and broader subfield rule families remain deferred). -* **FILE_TYPE** — ``file_id`` rules and Activity required messages/fields +* **FILE_TYPE** — ``file_id`` rules plus Activity and Workout required + messages/fields * **PRESERVATION** — opt-in checks for post-edit rewrite loss (e.g. unknown field ``raw_bytes`` cleared). Not part of default / strict levels. -File-type rules are implemented only for **Activity**. Other ``file_id.type`` -values fail closed at the FILE_TYPE level (intentional until more validators -exist). +File-type rules are implemented for **Activity** and **Workout**. Other +``file_id.type`` values fail closed at the FILE_TYPE level (intentional until +more validators exist, e.g. Course). """ from __future__ import annotations @@ -46,7 +47,10 @@ MAX_FIELD_COUNT = 255 # file_id.type values with a FILE_TYPE rule set implemented today. -IMPLEMENTED_FILE_TYPES = frozenset({FileType.ACTIVITY.value}) +IMPLEMENTED_FILE_TYPES = frozenset({ + FileType.ACTIVITY.value, + FileType.WORKOUT.value, +}) class ConformanceLevel(Enum): @@ -491,6 +495,21 @@ def _collect_file_type_findings( f'An activity FIT file requires exactly one activity message; found {activity_count}.', ) _collect_activity_field_findings(data_messages, findings, data_message_indices) + elif file_type == FileType.WORKOUT.value: + workout_count = message_counts[MesgNum.WORKOUT.value] + if workout_count != 1: + _error( + findings, + ConformanceLevel.FILE_TYPE, + f'A workout FIT file requires exactly one workout message; found {workout_count}.', + ) + if message_counts[MesgNum.WORKOUT_STEP.value] < 1: + _error( + findings, + ConformanceLevel.FILE_TYPE, + 'A workout FIT file requires at least one workout_step message.', + ) + _collect_workout_field_findings(data_messages, findings, data_message_indices) else: _error( findings, @@ -537,6 +556,37 @@ def _collect_activity_field_findings( ) +def _collect_workout_field_findings( + data_messages: Sequence[DataMessage], + findings: list[ValidationFinding], + data_message_indices: Mapping[int, int], +) -> None: + """Required fields for Workout FILE_TYPE (Garmin file-type + SDK samples). + + ``num_valid_steps`` is required on the workout message. Step count is not + cross-checked against that value: official SDK Workout fixtures sometimes + disagree (repeat meta-steps), and acceptance requires those fixtures to + validate clean. ``workout_session`` is optional (multi-sport only). + """ + required_fields = { + MesgNum.WORKOUT.value: ('num_valid_steps',), + MesgNum.WORKOUT_STEP.value: ( + 'message_index', + 'duration_type', + 'target_type', + ), + } + for message_pos, message in enumerate(data_messages): + field_names = required_fields.get(message.global_id) + if field_names is not None: + _require_fields_findings( + findings, + message, + field_names, + data_message_indices.get(message_pos), + ) + + def _collect_preservation_findings( records: Sequence[Record], findings: list[ValidationFinding], diff --git a/news/SHA-21.feature b/news/SHA-21.feature new file mode 100644 index 0000000..e53e66d --- /dev/null +++ b/news/SHA-21.feature @@ -0,0 +1,5 @@ +FILE_TYPE validation for **Workout** files: required `workout` / `workout_step` +messages and fields (`num_valid_steps`, step `message_index` / +`duration_type` / `target_type`). Activity behavior unchanged; other +`file_id.type` values (e.g. Course) still fail closed. SDK Workout fixtures +validate clean under FILE_TYPE. From 39be4071fa5a9082ebae3f5600408cec0f360c57 Mon Sep 17 00:00:00 2001 From: friendsa Date: Sat, 1 Aug 2026 13:35:33 +0000 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20Codex=20P2=20=E2=80=94=20workout=20n?= =?UTF-8?q?um=5Fvalid=5Fsteps=20and=20duration=5Fvalue=20checks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-check num_valid_steps against non-repeat workout_step count. Require duration_value (or duration_* subfield) for non-OPEN duration types. Co-authored-by: multica-agent --- fit_tool/tests/test_workout_files.py | 53 +++++++++++++++++ fit_tool/validation.py | 85 ++++++++++++++++++++++++++-- 2 files changed, 133 insertions(+), 5 deletions(-) diff --git a/fit_tool/tests/test_workout_files.py b/fit_tool/tests/test_workout_files.py index f1a8ca5..c36cdaa 100644 --- a/fit_tool/tests/test_workout_files.py +++ b/fit_tool/tests/test_workout_files.py @@ -103,3 +103,56 @@ def test_strict_builder_builds_minimal_workout(self): levels={ConformanceLevel.FILE_TYPE}, ) self.assertFalse(report.has_errors) + + def _minimal_workout_builder(self): + builder = FitFileBuilder(auto_define=True, min_string_size=50) + file_id = FileIdMessage() + file_id.type = FileType.WORKOUT + file_id.manufacturer = Manufacturer.DEVELOPMENT.value + file_id.product = 0 + file_id.serial_number = 0x12345678 + file_id.time_created = 1_700_000_000_000 + builder.add(file_id) + return builder + + def test_num_valid_steps_mismatch_errors(self): + """Codex P2: num_valid_steps must match non-repeat step count.""" + builder = self._minimal_workout_builder() + workout = WorkoutMessage() + workout.workout_name = 'Bad count' + workout.sport = Sport.CYCLING + workout.num_valid_steps = 0 # wrong — one ordinary step present + builder.add(workout) + step = WorkoutStepMessage() + step.message_index = 0 + step.duration_type = WorkoutStepDuration.TIME + step.duration_time = 60.0 + step.target_type = WorkoutStepTarget.OPEN + builder.add(step) + report = validate_fit_file(builder.build(), levels={ConformanceLevel.FILE_TYPE}) + self.assertTrue(report.has_errors) + self.assertTrue( + any('num_valid_steps' in f.message for f in report.errors), + [f.message for f in report.errors], + ) + + def test_value_duration_without_duration_value_errors(self): + """Codex P2: TIME (and other non-OPEN) requires duration_value.""" + builder = self._minimal_workout_builder() + workout = WorkoutMessage() + workout.workout_name = 'No duration' + workout.sport = Sport.CYCLING + workout.num_valid_steps = 1 + builder.add(workout) + step = WorkoutStepMessage() + step.message_index = 0 + step.duration_type = WorkoutStepDuration.TIME + # intentionally no duration_time / duration_value + step.target_type = WorkoutStepTarget.OPEN + builder.add(step) + report = validate_fit_file(builder.build(), levels={ConformanceLevel.FILE_TYPE}) + self.assertTrue(report.has_errors) + self.assertTrue( + any('duration_value' in f.message for f in report.errors), + [f.message for f in report.errors], + ) diff --git a/fit_tool/validation.py b/fit_tool/validation.py index 3b8efc6..d854aa9 100644 --- a/fit_tool/validation.py +++ b/fit_tool/validation.py @@ -34,7 +34,7 @@ from fit_tool.exceptions import FitValidationError from fit_tool.field import UnknownField from fit_tool.message import Message -from fit_tool.profile.profile_type import FileType, MesgNum +from fit_tool.profile.profile_type import FileType, MesgNum, WorkoutStepDuration from fit_tool.record import Record if TYPE_CHECKING: @@ -556,6 +556,26 @@ def _collect_activity_field_findings( ) +def _workout_step_duration_value(duration_type: Any) -> int | None: + """Normalize WorkoutStepDuration enum or raw int to its integer value.""" + if duration_type is None: + return None + if isinstance(duration_type, WorkoutStepDuration): + return int(duration_type.value) + try: + return int(duration_type) + except (TypeError, ValueError): + return None + + +def _is_repeat_workout_step_duration(duration_type: Any) -> bool: + """True for REPEAT_UNTIL_* meta-steps (not counted in ``num_valid_steps``).""" + value = _workout_step_duration_value(duration_type) + if value is None: + return False + return value >= int(WorkoutStepDuration.REPEAT_UNTIL_STEPS_CMPLT.value) + + def _collect_workout_field_findings( data_messages: Sequence[DataMessage], findings: list[ValidationFinding], @@ -563,10 +583,12 @@ def _collect_workout_field_findings( ) -> None: """Required fields for Workout FILE_TYPE (Garmin file-type + SDK samples). - ``num_valid_steps`` is required on the workout message. Step count is not - cross-checked against that value: official SDK Workout fixtures sometimes - disagree (repeat meta-steps), and acceptance requires those fixtures to - validate clean. ``workout_session`` is optional (multi-sport only). + ``num_valid_steps`` is required on the workout message and is cross-checked + against the count of non-repeat ``workout_step`` messages (repeat meta-steps + are excluded). ``workout_session`` is optional (multi-sport only). + + Value-based ``duration_type`` values (everything except ``OPEN``) require a + present ``duration_value`` / subfield payload. """ required_fields = { MesgNum.WORKOUT.value: ('num_valid_steps',), @@ -576,6 +598,7 @@ def _collect_workout_field_findings( 'target_type', ), } + valid_step_count = 0 for message_pos, message in enumerate(data_messages): field_names = required_fields.get(message.global_id) if field_names is not None: @@ -586,6 +609,58 @@ def _collect_workout_field_findings( data_message_indices.get(message_pos), ) + if message.global_id != MesgNum.WORKOUT_STEP.value: + continue + + record_index = data_message_indices.get(message_pos) + duration_type = getattr(message, 'duration_type', None) + duration_value = _workout_step_duration_value(duration_type) + + if not _is_repeat_workout_step_duration(duration_type): + valid_step_count += 1 + + # OPEN may omit duration_value; all other declared types need a value. + if ( + duration_value is not None + and duration_value != int(WorkoutStepDuration.OPEN.value) + ): + duration_payload = getattr(message, 'duration_value', None) + if duration_payload is None: + # Subfield getters (duration_time, etc.) may still surface a value. + duration_payload = getattr(message, 'duration_time', None) + if duration_payload is None: + _error( + findings, + ConformanceLevel.FILE_TYPE, + ( + f'{message.name} duration_type requires duration_value ' + f'(or a duration_* subfield); duration_type={duration_type!r}.' + ), + record_index, + ) + + for message_pos, message in enumerate(data_messages): + if message.global_id != MesgNum.WORKOUT.value: + continue + declared = getattr(message, 'num_valid_steps', None) + if declared is None: + continue + try: + declared_int = int(declared) + except (TypeError, ValueError): + continue + if declared_int != valid_step_count: + _error( + findings, + ConformanceLevel.FILE_TYPE, + ( + f'workout.num_valid_steps is {declared_int} but found ' + f'{valid_step_count} non-repeat workout_step message(s) ' + f'(repeat meta-steps excluded).' + ), + data_message_indices.get(message_pos), + ) + def _collect_preservation_findings( records: Sequence[Record],