SHA-22: FILE_TYPE Course 规则（J） - #56
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dc589fac7
ℹ️ 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".
| omit timestamps. Course name is the ``course_name`` property (field ``name``). | ||
| """ | ||
| required_fields = { | ||
| MesgNum.COURSE.value: ('course_name',), |
There was a problem hiding this comment.
When a Course message has course_name but leaves sport unset, FILE_TYPE validation reports no errors and FitFileBuilder(strict=True) serializes the file, even though sport is required Course metadata (and the repository's Course authoring path sets it). Include sport in this tuple so strict validation does not accept malformed Course files.
Useful? React with 👍 / 👎.
| has_timer_start = any( | ||
| _is_course_timer_event(message, frozenset({EventType.START.value})) | ||
| for message in data_messages | ||
| ) | ||
| has_timer_stop = any( | ||
| _is_course_timer_event(message, _COURSE_TIMER_STOP_TYPES) | ||
| for message in data_messages |
There was a problem hiding this comment.
When a timer stop precedes the timer start—or both events occur before all track records—these independent presence checks still accept the Course, so strict mode can emit a file whose timer interval does not delimit the course activity. Track the matching event positions/timestamps and require the start to precede the records and terminal stop.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56 +/- ##
==========================================
- Coverage 93.25% 92.83% -0.43%
==========================================
Files 35 35
Lines 2581 2625 +44
Branches 421 437 +16
==========================================
+ Hits 2407 2437 +30
- Misses 105 112 +7
- Partials 69 76 +7 ☔ View full report in Codecov by Harness. |
Rebased onto main after Workout (#55). Course structure + fields, course.sport required, timer start/stop ordering around track records. Co-authored-by: multica-agent <github@multica.ai>
d97750e to
960bf95
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 960bf95fd7
ℹ️ 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".
| # Ordering: timer start before track records and before terminal timer stop | ||
| # (message order; timestamps may be equal in real Garmin exports). | ||
| if start_positions and stop_positions: | ||
| first_start = min(start_positions) | ||
| last_stop = max(stop_positions) |
There was a problem hiding this comment.
When messages are physically ordered start → records → stop but their timestamps are reversed, such as start=200 and stop=100, all three position checks pass and strict mode accepts a chronologically impossible timer interval. Equal timestamps can remain legal while still requiring the start timestamp to be no later than the terminal stop timestamp. Fresh evidence beyond the earlier ordering comment is that this newly added block explicitly checks only message positions even though timer timestamps are required.
Useful? React with 👍 / 👎.
| def add_minimal_course_messages(builder, *, include_course=True, include_lap=True, | ||
| include_record=True, include_timer_start=True, | ||
| include_timer_stop=True, course_name='test course'): |
There was a problem hiding this comment.
Adding the Course helper here replaces add_minimal_workout_messages rather than supplementing it, and the same diff deletes the positive strict/report Workout tests, the missing-workout/step test, and the missing-field case. Consequently, no test now exercises successful or failed Workout FILE_TYPE validation, so a regression to the SHA-21 behavior can pass the full suite; retain the Workout helper and cases alongside the new Course coverage.
AGENTS.md reference: AGENTS.md:L144-L145
Useful? React with 👍 / 👎.
| | **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 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) | | ||
| | **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, Workout, and Course** 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) | |
There was a problem hiding this comment.
After this row advertises Course FILE_TYPE validation as implemented, the linked docs/FIT_CONFORMANCE_DESIGN.md still states at lines 25–26, 50, 542–555, and 741–742 that Course validation is a gap and fails closed. This leaves the repository's status source directly contradicting the newly published capability matrix; update those entries when flipping the README claim, as the design document itself requires at line 51.
Useful? React with 👍 / 👎.
1 participant
Summary
file_id.type = Course(exactly onecourse, ≥1lap, ≥1record, timer start + stop events).course_name; lap summary fields; record positions (track geometry; timestamps optional); event timer fields; Coursefile_idwithout requiringproduct(Stages Link omits it).stagesLink_28832.fitfixture.news/SHA-22.feature.Multica
Test plan
uv run python -m pytest fit_tool/tests/ -q— 419 passed, 1 skippedstagesLink_28832.fitpasses FILE_TYPE