Skip to content

fix(peloton): accept nullable workout fields - #2044

Merged
Asherlc merged 39 commits into
mainfrom
Asherlc/use-subagents
Jul 26, 2026
Merged

Asherlc merged 39 commits into
mainfrom
Asherlc/use-subagents

Conversation

@Asherlc

@Asherlc Asherlc commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Summary

  • accept the exact nullable/omitted workout fields observed in five consecutive Peloton production responses
  • preserve upstream nulls at the client boundary and normalize nullable completion/source IDs only in the parsed DTO
  • add production-shaped client and parser regressions
  • record the incident evidence and validation

Fixes DOFEK-SERVER-5E.

Root cause

The runtime response schema modeled several optional Peloton fields as undefined-only and required end_time plus ride.instructor.id. Peloton returned JSON null for title, end_time, metrics_type, peloton_id, and strava_id, and omitted the instructor ID, so Zod rejected the whole workout page before parsing.

Validation

  • test first failed with the exact six Sentry validation paths
  • 17 focused client/parser tests pass
  • pnpm --filter @dofek/peloton typecheck
  • targeted Biome checks pass
  • parser mutation test: 13/13 mutants killed
  • full Docker-free suite: 849 files passed, 2 skipped; 13,852 tests passed, 21 skipped

The schema change is limited to observed upstream absence semantics; it adds no fallback request, compatibility route, feature flag, or one-off normalization path.


Summary by cubic

Accepts nullable Peloton workout fields and missing instructor IDs at the client boundary, and normalizes null completion/source IDs during parsing to match production payloads and unblock scheduled syncs. Fixes DOFEK-SERVER-5E.

  • Bug Fixes
    • Zod schema: title, metrics_type, peloton_id, and strava_id are now nullish; end_time is nullable; ride.instructor.id is optional.
    • Parser: treat end_time: null as absent; ignore null/“-1” Strava and Peloton class IDs.
    • Tests/docs: add production-shaped client/parser regressions, share the response schema in provider contract tests, and record the incident evidence in the ops baseline.

Written for commit e86e116. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of workout responses with nullable fields, missing instructor IDs, and omitted values.
    • Prevented invalid end dates and identifiers from appearing when source data is unavailable.
    • Restored reliable workout synchronization for affected Peloton responses.
  • Tests

    • Added coverage for nullable and incomplete workout data.
    • Updated API contract validation to use the shared workout response definition.
  • Documentation

    • Added a production incident baseline entry documenting the issue, resolution, validation, and follow-up actions.

Asherlc added 30 commits July 25, 2026 22:42
Reuse activity summary freshness and cap dirty-key batches so analytics cycles do not expand the entire power backlog. Consume precomputed normalized power during refits.
# Conflicts:
#	docs/production-incident-baseline.md
Treat partial and runtime failures as failed while allowing intentional no-op results. Fixes DOFEK-SERVER-5D.
Avoid recursive ClickHouse views on the request path and verify the compact-model query against a real database. Fixes DOFEK-SERVER-5C.
- time out provider HTTP requests after two minutes
- continue provider deletion one durable batch at a time
- align Swarm grace and deploy convergence windows

Refs DOFEK-SERVER-4N
Refs DOFEK-SERVER-2K
# Conflicts:
#	docs/production-incident-baseline.md
# Conflicts:
#	.github/workflows/deploy-web-stack.yml
#	docs/production-incident-baseline.md
Copilot AI review requested due to automatic review settings July 26, 2026 17:08
@cursor

cursor Bot commented Jul 26, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry @Asherlc, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codereviewbot-ai

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b6b72aa0-8a8f-4c74-b67a-1ace68ab456d

📥 Commits

Reviewing files that changed from the base of the PR and between d627cd8 and e86e116.

📒 Files selected for processing (6)
  • docs/production-incident-baseline.md
  • packages/peloton-client/src/client.test.ts
  • packages/peloton-client/src/parsing.test.ts
  • packages/peloton-client/src/parsing.ts
  • packages/peloton-client/src/types.ts
  • src/providers/provider-api-contracts.test.ts

📝 Walkthrough

Walkthrough

Peloton schemas now accept nullable or omitted workout fields and instructor IDs. Workout parsing normalizes null completion and source identifiers to undefined. Client and provider contract tests cover the response shape, and an incident baseline documents the production drift and remediation.

Changes

Peloton response drift handling

Layer / File(s) Summary
Schema contract and client validation
packages/peloton-client/src/types.ts, packages/peloton-client/src/client.test.ts, src/providers/provider-api-contracts.test.ts
Peloton instructor and workout schemas accept nullable or omitted fields, while client and provider tests validate the updated workout response contract.
Workout parsing and incident validation
packages/peloton-client/src/parsing.ts, packages/peloton-client/src/parsing.test.ts, docs/production-incident-baseline.md
Workout parsing maps null completion and source identifiers to undefined, with focused test coverage and a documented production incident baseline.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested labels: area/providers, type/bug

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title is relevant, but it violates the required area-prefix format by using fix(peloton) instead of a bracketed prefix. Use a bracketed area prefix and imperative phrasing, e.g. "[peloton-client] accept nullable workout fields".
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Peloton: accept nullable workout fields and missing instructor IDs

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

AI Description

• Relax Peloton workout response validation to accept observed nulls and omitted instructor IDs.
• Preserve upstream nulls at the client boundary; normalize only inside parsed workout DTOs.
• Add production-shaped regressions and record incident evidence/validation steps.
Diagram

graph TD
  A{{"Peloton API"}} --> B["PelotonClient"] --> C["Zod schemas (types.ts)"] --> D["parseWorkout"] --> E["Parsed workout DTO"]
  T1["client.test.ts"] --> B
  T2["parsing.test.ts"] --> D
  DOC["Incident baseline doc"] --> C
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Pre-normalize nulls before schema validation
  • ➕ Keeps schema strict/unchanged while tolerating upstream nulls
  • ➕ Centralizes absence-shimming in one place
  • ➖ Mutates/obscures the raw upstream payload at the boundary
  • ➖ Harder to distinguish real upstream nulls vs local rewriting during incident response
2. Broaden schema significantly (partial/loose validation)
  • ➕ More resilient to future Peloton drift without frequent schema edits
  • ➖ Increases risk of silently accepting truly malformed payloads
  • ➖ Pushes failures deeper into parsing/consumer code with worse debugging signals
3. Introduce a compatibility fallback path/feature flag
  • ➕ Enables controlled rollout and quick revert if assumptions are wrong
  • ➖ Adds long-lived complexity and branching behavior for a narrow schema correction
  • ➖ Harder to keep both paths equally tested

Recommendation: Keep the PR’s approach: update the boundary schema to match observed upstream absence semantics (null + omission) while preserving the raw payload, and normalize only in the parsed DTO. This maintains strong validation for required identity/timestamp fields, fixes the immediate sync blocker, and keeps debugging/incident evidence accurate without introducing a secondary compatibility path.

Files changed (5) +93 / -7

Bug fix (2) +8 / -7
parsing.tsHandle nullable end_time when computing endedAt +2/-1

Handle nullable end_time when computing endedAt

• Adjusts workout parsing to treat a null end_time as zero and preserve the existing unfinished-workout behavior by emitting an undefined endedAt.

packages/peloton-client/src/parsing.ts

types.tsRelax Peloton Zod schemas to accept nullish fields +6/-6

Relax Peloton Zod schemas to accept nullish fields

• Updates the runtime workout schema to accept nullish title/metrics_type/peloton_id/strava_id and a nullable end_time, and makes instructor.id optional to match observed upstream omission.

packages/peloton-client/src/types.ts

Tests (2) +52 / -0
client.test.tsAdd client regression for nullable workout fields +39/-0

Add client regression for nullable workout fields

• Introduces a production-shaped workouts response fixture with null title/end_time/IDs and a missing instructor ID, asserting the client accepts the payload without schema rejection.

packages/peloton-client/src/client.test.ts

parsing.test.tsAdd parser regression for nullable completion/source IDs +13/-0

Add parser regression for nullable completion/source IDs

• Adds a test ensuring null end_time, peloton_id, and strava_id are treated as absent in the parsed DTO (undefined).

packages/peloton-client/src/parsing.test.ts

Documentation (1) +33 / -0
production-incident-baseline.mdDocument Peloton response drift incident and validation +33/-0

Document Peloton response drift incident and validation

• Adds an incident entry describing the upstream null/omission drift that broke workout sync, the exact schema mismatches, and the validation checklist for closing the Sentry issue.

docs/production-incident-baseline.md

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Storybook previews for 111768d5 are ready:

This comment updates automatically on each PR push.

@qodo-code-review

qodo-code-review Bot commented Jul 26, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 154 rules

Grey Divider


Remediation recommended

1. Contract test schema drift ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The Peloton client workout schema now accepts end_time: null, but the Peloton API contract test
still requires end_time to be a number. This makes the contract test disagree with the runtime
client boundary and can fail when Peloton returns the observed nullable end_time.
Code

packages/peloton-client/src/types.ts[R24-28]

+  title: z.string().nullish(),
  created_at: z.number(),
  start_time: z.number(),
-  end_time: z.number(),
+  end_time: z.number().nullable(),
  total_work: z.number(),
Relevance

⭐⭐⭐ High

Repo commonly updates Zod schemas to reflect real nullability; mismatched validations/tests are
treated as a bug to align.

PR-#1422
PR-#2015

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR explicitly changes the runtime schema to allow a nullable end_time, but the Peloton
contract test continues to assert that end_time is always a number, so the two validations no
longer match and the contract test can reject payloads the client now intentionally accepts.

packages/peloton-client/src/types.ts[19-43]
src/providers/provider-api-contracts.test.ts[412-424]
PR-#2015

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Peloton API contract test schema still requires `end_time` to be a number, while the runtime client schema now accepts `end_time: null`. This causes schema drift between “contract tests” and the real client boundary, reducing the usefulness of the contract test and potentially failing when run against production-shaped responses.

## Issue Context
- Runtime client schema: `packages/peloton-client/src/types.ts` now uses `end_time: z.number().nullable()`.
- Contract test schema: `src/providers/provider-api-contracts.test.ts` still uses `end_time: z.number()`.

## Fix Focus Areas
- packages/peloton-client/src/types.ts[19-43]
- src/providers/provider-api-contracts.test.ts[412-424]

## Suggested fix
- Update the contract test’s `pelotonWorkoutSchema.end_time` to `z.number().nullable()`.
- Consider reusing/importing the canonical schema from `@dofek/peloton/types` (or exporting a shared schema) to avoid future drift.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Peloton nullability claim lacks citation ✓ Resolved 📘 Rule violation § Compliance
Description
The new incident entry states specific Peloton API response behavior (nullable fields and omitted
instructor ID) without an adjacent primary/official source link in the same paragraph. This violates
the documentation requirement to cite third-party behavior claims, reducing auditability of the
claim.
Code

docs/production-incident-baseline.md[R18418-18425]

+- **Evidence:** Five consecutive production events from `14:30` through
+  `16:30` UTC reported the same response drift. Peloton returned `null` for
+  workout `title`, `end_time`, `metrics_type`, `peloton_id`, and `strava_id`,
+  and omitted `ride.instructor.id`; the runtime schema accepted only omitted
+  optional strings and required a numeric end time and instructor ID.
+- **Root cause:** The client boundary modeled several optional upstream fields
+  as undefined-only even though the observed API payload uses both JSON `null`
+  and omission for absence. The schema therefore rejected valid unfinished and
Relevance

⭐⭐⭐ High

Docs incident entries are expected to add primary-source links for behavior/evidence claims; this
exact feedback is usually accepted.

PR-#1858
PR-#1864
PR-#1677

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 1505719 requires third-party behavior claims in docs/ to include an adjacent
official/primary-source link in the same paragraph or immediately following line. The added
paragraph describing Peloton returning null for several fields and omitting ride.instructor.id
contains no such link.

Rule 1505719: Cite third-party behavior claims in docs with primary sources
docs/production-incident-baseline.md[18418-18425]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A new doc section claims specific Peloton API response behavior (fields returned as `null`, omitted `ride.instructor.id`) but does not include an adjacent citation in the same paragraph/immediately following line.

## Issue Context
The compliance rule requires third-party behavior claims in `docs/` to be backed by a nearby hyperlink/reference to an official or primary source. For private/undocumented endpoints, a repo-stored primary artifact (e.g., a sanitized captured response JSON) can serve as the primary source.

## Fix Focus Areas
- docs/production-incident-baseline.md[18418-18425]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread docs/production-incident-baseline.md Outdated
Comment thread packages/peloton-client/src/types.ts
@codereviewbot-ai

Copy link
Copy Markdown

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.

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.

2 participants