test(shared): property tests + scoped StrykerJS mutation testing (#417, #418) - #427
Conversation
#418) #418 — @fast-check/vitest property tests (seed 424242, 100 runs each) over the pure cross-platform logic: fallbackChildOverdue overdue spec, buildStreakWeekDays streak contiguity, optimisticPatchHabit idempotence/identity/inverse, and habitFormSchema+ goalFormSchema re-parse fixed point. Export fallbackChildOverdue as the test seam (still used internally by normalizeChildren). #417 — StrykerJS 9.6.1 + vitest-runner, stryker.config.json scoped to the pure validation/utils logic in incremental mode with thresholds.break=null (report-only this round; a follow-up flips it under the captured baseline). Adds mutation.yml (per-PR incremental report + nightly full-scope report), a direct nowDate test, and .gitignore entries. Closes #417 Closes #418 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR 427
Scope: PR #427 in thomasluizon/orbit-ui-mobile — test(shared): property tests + scoped StrykerJS mutation testing (#417, #418)
Recommendation: APPROVE
Summary
Bundles two test-infrastructure issues: property-based tests (@fast-check/vitest) over pure packages/shared logic (fallbackChildOverdue, buildStreakWeekDays, optimisticPatchHabit, habitFormSchema/goalFormSchema), plus a new report-only StrykerJS mutation-testing CI workflow scoped to packages/shared's pure validation/utils logic. No apps/web, apps/mobile, API, i18n, or contract-surface code is touched. Every new property test was traced against the implementation it exercises and the assertions match actual behavior; the CI workflow is consistent with the repo's existing Node-version and trigger conventions.
Findings
Critical
None
High
None
Medium
None
Low / Info
[Info] Newly-exported test seam duplicates an existing private helper
· dimension: 2 (Dead/stale code) / 10 (DRY)
· location: packages/shared/src/utils/habit-normalization.ts:63 (export change) vs packages/shared/src/utils/drill-navigation.ts:40 (pre-existing, unchanged private fallbackChildOverdue)
· issue: habit-normalization.ts's fallbackChildOverdue is now exported as a test seam, but drill-navigation.ts already has its own private function of the same name and apparently the same semantics — pre-existing duplication this PR doesn't touch.
· risk: None from this PR; flagged only because the new export makes the duplicate more visible. Pre-existing, not Critical — out of this diff's rubric scope per "focus on changed code."
· fix: Not for this PR. Worth a lightweight follow-up to consolidate both into the one exported habit-normalization.ts version.
· reference: CLAUDE.md rule 10 (DRY at the right level)
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or packages/shared/src/i18n/*.json changed |
| contract-aligner | N/A — no packages/shared/src/types/* / endpoints.ts changed; orbit-api not touched |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* or landing-page UI file changed |
None of the five subagents were invoked — every gate in the rubric's Phase 4 table failed to trigger for this diff.
Validation
| Check | Result |
|---|---|
| Lint | N/A — CI adaptation: skipped (Build/Unit Tests/SonarCloud run as separate required checks on this PR) |
| Type check | N/A — same; PR body self-reports 3/3 passing |
| Tests | N/A — same; PR body self-reports 1420 shared tests green |
| Build (api) | N/A — orbit-api not touched, sibling repo not checked out in this job |
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop): N/A — no
apps/*UI files touched. - Dimension 9 (Parity): N/A — no
apps/web/**/apps/mobile/**file changed. - Dimension 10 (i18n): N/A — no user-facing strings or locale files touched.
- Dimension 11 (Contract drift): N/A — no
packages/shared/src/types/*/endpoints.tsor orbit-api DTO changed; siblingorbit-apirepo isn't checked out in this CI job regardless. - Dimension 13 (Backend hard rules): N/A —
orbit-apinot touched. - Dimension 14 (FEATURES.md parity): N/A — pure test/CI infra, no user-facing feature surface changed.
- Validate (Phase 7): skipped per this workflow's CI adaptation.
- package-lock.json (1,296 additions / 78 deletions): not reviewed line-by-line (autogenerated); spot-checked that new top-level entries (
@fast-check/vitest,@stryker-mutator/core,@stryker-mutator/vitest-runner) correspond to the declared devDependency additions. No fullnpm auditrun. - All 10 non-lockfile changed files were read in full and given a verdict — nothing else silently skipped.
- Prior-review check: no earlier Claude review or unresolved human review thread exists on this PR (only a
github-advanced-securityempty-body COMMENTED review and a Vercel bot comment) — nothing to avoid re-flagging.
What's good
- Every property test's invariant was checked against the actual implementation it targets and matches real behavior — none asserts something the code doesn't do.
- Seeded, fixed
numRuns: 100params keep the property tests deterministic and reproducible. - The mutation workflow correctly starts
break: null(report-only) with a stated follow-up to flip it blocking — an honest incremental rollout. stryker.config.json'smutateglobs all resolve to real, existing files.- CI workflow's
node-version: 22andpull_request-only trigger (nopull_request_target) match the rest of.github/workflows/*.yml— no secret-exposure risk from fork PRs. - The one private→exported change (
fallbackChildOverdue) is scoped, justified in the PR body as a deliberate test seam, and doesn't alter runtime behavior.
Recommendation
Clean, low-risk test-infrastructure PR — approve as is. The single Info-level note (pre-existing fallbackChildOverdue duplication, surfaced by but not created by this PR) is worth a lightweight follow-up issue, not a blocker.
Resolves the CodeQL missing-permissions alert. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review: PR 427 (re-review after new commit)
Scope: PR #427 in thomasluizon/orbit-ui-mobile — test(shared): property tests + scoped StrykerJS mutation testing (#417, #418)
Recommendation: APPROVE
Summary
Full re-walk of the current diff (head 9ee67da2) against the rubric. The bulk of the
PR — property-based tests over packages/shared pure logic (fallbackChildOverdue,
buildStreakWeekDays, optimisticPatchHabit, habitFormSchema/goalFormSchema) plus
the report-only StrykerJS mutation workflow — was already reviewed and approved at
commit 5c4f4796; every assertion was re-traced against its implementation again here
and still matches actual behavior. The only change since that review is one follow-up
commit, 9ee67da2, which adds permissions: contents: read at the workflow level to
.github/workflows/mutation.yml to close a CodeQL missing-permissions alert. Neither
job in the workflow declares its own permissions block, so both incremental and
nightly correctly inherit the new read-only, least-privilege grant — this is a pure
hardening addition with no behavior change and no new risk.
Findings
Critical
None
High
None
Medium
None
Low / Info
[Info] Newly-exported test seam duplicates an existing private helper
· dimension: 2 (Dead/stale code) / 10 (DRY)
· location: packages/shared/src/utils/habit-normalization.ts:63 (export change) vs packages/shared/src/utils/drill-navigation.ts:40 (pre-existing, unchanged private fallbackChildOverdue)
· issue: carried over from the prior review pass — pre-existing duplication this PR doesn't touch, only made more visible by the new export.
· risk: None from this PR.
· fix: Not for this PR; worth a lightweight follow-up to consolidate both into the one exported habit-normalization.ts version.
· reference: CLAUDE.md rule 10 (DRY at the right level)
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or packages/shared/src/i18n/*.json changed |
| contract-aligner | N/A — no packages/shared/src/types/* / endpoints.ts changed; orbit-api not touched |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* or landing-page UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — not re-run this pass; PR body self-reports 3/3 passing and no source logic changed since |
| Type check | N/A — same |
| Tests | N/A — same; PR body self-reports 1420 shared tests green |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- Dimensions 8, 9, 10, 11, 13, 14: N/A, same gating as the prior pass (no
apps/*, i18n, contract-surface,orbit-api, or feature-surface files touched). package-lock.json: not re-diffed line-by-line (autogenerated, unchanged by the delta commit).- The delta commit (
9ee67da2, 3 lines, one file) was read in full and given a verdict.
What's good
- The permissions fix is scoped, minimal, and targets exactly the CodeQL finding it names — no scope creep.
- Read-only workflow-level
permissionsis the correct least-privilege default for a job that only checks out code and uploads artifacts.
Recommendation
Approve. No action required beyond the previously-noted non-blocking follow-up on the fallbackChildOverdue duplication.
There was a problem hiding this comment.
Code Review: PR #427 — test(shared): property tests + scoped StrykerJS mutation testing (#417, #418)
Scope: PR #427 in thomasluizon/orbit-ui-mobile (bundles #417 + #418)
Recommendation: APPROVE
Summary
Test-and-tooling-only PR: four new @fast-check/vitest property-test files over pure packages/shared logic (fallbackChildOverdue, optimisticPatchHabit, buildStreakWeekDays, habitFormSchema/goalFormSchema), a StrykerJS 9.6.1 mutation-testing setup (report-only, break: null), and the supporting CI workflow / .gitignore / package.json changes. No apps/*, packages/shared/src/types/*, endpoints.ts, or orbit-api files are touched, so the parity, i18n, contract-drift, design, and backend-hard-rules dimensions are all N/A this round. I traced each new property test's math against its production function (fallbackChildOverdue's date-boundary logic, optimisticPatchHabit's idempotence/inverse claims, buildStreakWeekDays's contiguous-active-window formula, and the Zod schemas' trim/default fixed-point) and found the assertions correct and non-tautological.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
- Info:
fallbackChildOverdueis now exported fromhabit-normalization.tspurely as a test seam; note (not a blocker) that an identical private copy already exists indrill-navigation.ts— pre-existing duplication from before this diff, left as-is per the rubric's "changed code only" scope. - Info: the
mutation.ymlincremental-cachekeyis keyed ongithub.run_id(always unique), sorestore-keys: stryker-incremental-is what actually provides incremental restore across runs; functions as intended, just worth knowing when tuning cache hit rate later.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no i18n/user-facing string changes |
| contract-aligner | N/A — no packages/shared/src/types/* / endpoints.ts / cross-repo change |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* UI files changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — covered by this PR's own required CI checks (Build/Unit Tests/SonarCloud) |
| Type check | N/A — covered by this PR's own required CI checks |
| Tests | N/A — covered by this PR's own required CI checks |
| Build (api) | N/A — orbit-api not touched, not checked out in this job |
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop), 9 (Parity), 10 (i18n), 11 (Contract drift/backward-compat), 13 (Backend hard rules), 14 (FEATURES.md parity) — all N/A, surface not touched by this diff (confirmed via file list, not assumed).
- Backward-compat guard (Phase 5): no candidates — no
packages/shared/src/types/*.tsor orbit-api DTO hunks in the diff. - All 11 changed files (workflow,
.gitignore,package-lock.json,packages/shared/package.json, 4 new property-test files,utils.test.ts,habit-normalization.ts,stryker.config.json) received a verdict above — nothing else deferred.
What's good
- Property tests target real invariants (date-boundary correctness, tree-patch idempotence/inverse, streak-window contiguity, schema re-parse fixed points) rather than restating the implementation — genuinely useful regression coverage.
- Stryker rollout is appropriately cautious: incremental per-PR + nightly full-scope, both report-only (
break: null) with an explicit stated follow-up to flip blocking once a baseline is captured — avoids an unvetted mutation-score gate blocking merges on day one. stryker.config.json'smutatescope is deliberately narrowed to pure validation/utils logic (all 7 referenced files verified to exist) rather than the whole package, keeping runtime bounded.- Workflow permissions are least-privilege (
contents: readonly), and Node version (22) matches every other workflow in the repo. - No engines bump — new dev dependencies' peer/engine requirements (vitest ^4.1.0, node >=20) are already satisfied by the existing toolchain, exactly as the PR body claims.
Recommendation
Nothing blocking. Safe to merge.
|
2 participants
Bundles #417 + #418 (correlated shared-testing hardening).
#418 — property-based tests (
@fast-check/vitest)Invariant tests over pure
packages/sharedlogic: overdue-never-true-for-future-DueDate, streak contiguity (buildStreakWeekDays),optimisticPatchHabitidempotence/identity/inverse, and validation-schema re-parse fixed point. Seed-pinned, 100 runs each. Exports the privatefallbackChildOverdueas the test seam. (Skip=Value=0 lives on the backend — covered by orbit-api#299.)#417 — scoped mutation testing (StrykerJS)
StrykerJS 9.6.1 + vitest-runner,
mutatescoped to the pure validation/utils logic. Per-PR incremental + nightly full-scope, both report-only (break: null) this round; a follow-up flipsbreakblocking under the CI-captured baseline. No engines bump (real floor is Node ≥20).Validated: type-check 3/3, lint 3/3, shared 1420 tests green (coverage above thresholds). Stryker config runs end-to-end.
🤖 Generated with Claude Code