Test suite audit

.pi/orksorksorks/alanvardy-var-1075-test-suite-audit/conventions.md

@@ -0,0 +1,62 @@
+# Conventions — SingleThread test/build appendix
+
+Shared factual reference for Design/Structure/Plan. All commands run from
+repo root `/Users/vardy/dev/alanvardy-var-1075-test-suite-audit`.
+
+## Canonical commands (Makefile + scripts/)
+
+- `make build` — iOS build. `make watch-build` — watch build.
+- `make test` — unit gate (`scripts/test.sh --unit-only`). `make ui-test` — UI gate (`--ui-only`). `make check` — full CI-identical gate (`scripts/test.sh`).
+- `make lint` — `swiftformat --lint` + `swiftlint lint --strict`. `make format` — `swiftformat` + `swiftlint --fix`.
+- `make periphery` — `periphery scan --strict` (reads stale DerivedData index — clean `DerivedData/` and rerun after branch switch).
+- `make coverage` / `coverage-ui` / `coverage-all`. `make reset-storekit`. `make clean`.
+- Gate pipeline order (`scripts/test.sh`): format → swiftlint → warning-check self-test → iOS build-for-testing → resolve/pin watch UDID + preboot → watch build → periphery → iOS UI tests → watch UI build+test+test → watch unit tests → macOS unit tests (`-only-testing:SingleThreadTests` on macOS native).
+- Single test: `scripts/test-one.sh <Target/Suite/case>` (exits non-zero on zero-match — a zero-match `-only-testing:` prints `** TEST SUCCEEDED **` and exits 0).
+
+## Destination pinning
+
+- Precedence: explicit `SIM=` > this worktree's `.simulator_id` > shared default `name=iPhone 17` (`Makefile:1-5`, `scripts/test.sh:8-76`).
+- Watch UI tests use `WATCH_TEST_SIM` (unpaired watch pinned by UDID).
+- Version floors (`scripts/test.sh:143-152`): iOS 17.0, watchOS 11.0, macOS 26.5.
+- One `xcodebuild test` process at a time; on `Busy`/`RequestDenied` shut down sims + kill orphaned `xcodebuild`/`xctest` (simulator-pairing skill).
+
+## Test-suite inventory & platform gating
+
+### SingleThreadTests/ — unit (Swift Testing, `import Testing`, `@Test`), runs on iOS AND macOS
+The single combined iOS+macOS unit suite; platform via whole-file/inline `#if os(...)`.
+- Whole-file gated (line 1): `os(iOS)` — `AppDelegateTests.swift:1`, `BackgroundCardTests.swift:8`, `AISortFailureBannerTests.swift:1`; `os(macOS)` — `MenuBarExtraPreferenceTests.swift:1`, `MacOSActionButtonChromeTests.swift:1`; `os(iOS) || os(watchOS)` — `RescheduleSyncTests.swift:1`, `SkippedReminderSyncServiceTests.swift:1`, `EntitlementSyncTests.swift:1`, `AppLanguageSyncTests.swift:1`, `EnableActionButtonsSyncTests.swift:1`.
+- Inline `#if os(macOS)`: `SettingsViewTests.swift:82,209,293,433`, `SettingsSubscreenLayoutTests.swift:13/25`, `AboutViewTests.swift:26`, `MicrophoneToggleTests.swift:196/234`.
+- `TestFixtures.swift:70` gates `FakeSession` behind `os(iOS) || os(watchOS)`.
+- Known local-only macOS failures (don't debug): `EntitlementStoreTests.isEntitledSurvivesStoreRecreation`, `initialRefreshSettlesResolvedFlag`, `hostStoreKitIsClean` (CI mac green).
+- Coverage highlights: `ReminderStoreTests.swift` (74 `@Test`, ~1300 lines, 90+ InMemoryEventStore injections, private `CompletedReturningEventStore` fake at `:1086`); `ReminderSkipTests.swift` (+ `ReminderSortTests` struct); `AISortCoordinatorTests.swift`; `AppGroupTests.swift` (`defaultsIsAStableInstance` guard); `UITestingSeedTests.swift`.
+
+### SingleThreadUITests/ — iOS UI (XCTest, XCUIApplication, `[--ui-testing]`) at `SingleThreadUITests.swift:28`
+Single file; launch/render smoke + `testAccessibilityAudit` (`performAccessibilityAudit`). SwiftFormat-excluded; keeps `test…` names. Empty `#else` mac branch so bundle compiles on macOS.
+
+### SingleThreadWatchTests/ — watch unit (Swift Testing), separate watch scheme, 0 whole-file gates
+7 files: `WatchAppViewModelTests.swift`, `WatchReminderViewModelTests.swift`, `WatchReminderViewRegressionTests.swift`, `WatchSyncPipelineTests.swift` (`:13-406`), `ReminderStoreWatchTests.swift`, `ShowCompletionGlowStateTests.swift`, `ShowEnableActionButtonsStateTests.swift`, + `TestFixtures.swift:1-7` (`sharedWatchEventStore`, reminder builder).
+
+### SingleThreadWatchUITests/ — watch UI (XCTest, `[--ui-testing]`) at `SingleThreadWatchUITests.swift:35`
+Single file (NOT SwiftFormat-excluded; still keeps XCTest `test…` names).
+
+## Core seams referenced by tests
+
+- `EventKitStoring.swift:8` protocol seam (`:45` `EKEventStore: EventKitStoring` adapter; non-watchOS methods `#if !os(watchOS)` `:49-63`).
+- `InMemoryEventStore.swift:13` in-memory fake (deps `reminders, calendars, deliverCompletionOffMain, saveError, defaultCalendar` `:17-32`; records `requestFullAccessCallCount:40`, `saveCallCount:42`; `saveError:44`; mirrors `predicateForIncompleteReminders` `:84`).
+- `AppGroup.defaults` (`AppGroup.swift:27-30`) single cached `UserDefaults` instance — shared-with-watch persistence MUST round-trip here (never `UserDefaults.standard`); guarded by `defaultsIsAStableInstance`.
+- Launch-arg seams (`AppViewModel.swift:231-317`): `--seed '<json>'` (via `UITestingSeed.fromLaunchArguments`, `:246`), `--ui-testing` (+ `-glow/-reduced-glow/-noop-settle/-app-language <raw>/-notifications`), `--no-reminders`. Watch variants in `WatchAppViewModel.swift:7-170,295`.
+- `ReminderStore.swift:22-45` single injectable init (`eventStore`, `loadsReminders:false`, pre-seeded `reminders/skippedIDs/pendingCompletions/authorizationStatus/excludedListTitles/hasHidden`, `settle` hook no-op in tests; production settle = 200 ms sleep). ReminderStore has NO AppGroup param (persistence via injected stores).
+
+## Build/verify gotchas
+
+- `SWIFT_TREAT_WARNINGS_AS_ERRORS = YES` project-wide; scope per-target overrides in pbxproj, never CLI (conflicts with SPM `-suppress-warnings`). Gate fails on any source-located compiler warning.
+- SwiftLint `--strict` in CI — every warning is an error; `swiftlint lint --strict` before commit.
+- Unit-test names must NOT start with `test`/`testing` (SwiftFormat strips them under `make format`); UI-test names keep `test…`. Variable names ≥ 3 chars (`identifier_name` exceptions: `id`, `e`, `d`, `rt`, `to`, `gvm`).
+- Force-unwrapping banned outside test code; test fixtures relax via `SingleThreadTests/.swiftlint.yml`.
+- New `.swift` file needs no pbxproj edit (synchronized groups). A **new test target** needs pbxproj object IDs, scheme TestAction wiring, `-only-testing` entries in `scripts/test.sh`, `Makefile test` target, and CI matrix entries.
+- Pre-existing failure on `origin/main` (diff didn't touch) → verify via `git blame`/CI; never `git stash` to baseline (spans branches) — use `git show origin/main:<path>` or throwaway worktree.
+- Watch UI runner needs `lib_TestingInterop.dylib` embedded locally (watch-UI stage) — handled by `scripts/test.sh`; XCTest runtimes pruned via `cleanup_xctest_runtimes`.
+- Local Xcode 27.0 vs CI 26.6 can diverge on `$`-projection-only `@State` (periphery) — see periphery skill.
+- `@MainActor` isolation is project-wide only on iOS app + watch app targets; Core/widget/test targets need explicit `@MainActor` annotations where needed.
+- Persisted values shared with the watch must round-trip through `AppGroup.defaults`; `--seed`/`--ui-testing` seams included.
+- Gate stages: phase subagents verify with build + targeted `-only-testing:` suites only; full CI-identical gate runs ONCE via the run-gate skill after phases commit — never nohup it ad-hoc.

.pi/orksorksorks/alanvardy-var-1075-test-suite-audit/design.md

@@ -0,0 +1,171 @@
+# Design Discussion — Test-Suite Audit (VAR-1075)
+
+## Current State
+
+The repo has a three-layer, well-seamed architecture. Research found the
+seams are already strong; the gaps are coverage, not structure.
+
+- **Core pure logic** (`ReminderSkip.swift:18`, `:56`, `:131`;
+  `ReminderSort.swift:5`; `AIReminderRanking.swift:36`, `:77`, `:123-157`)
+  is EK-free and directly unit-testable.
+- **Stateful `ReminderStore`** (`ReminderStore.swift:16`) owns the
+  `EKEventStore` behind `any EventKitStoring` (`:17-78`), with every
+  persistence store injected and a no-op `settle` hook for tests.
+- **Composition roots** (`AppViewModel.swift:17,31-69`;
+  `WatchAppViewModel.swift:10`) select real vs. in-memory stores from launch
+  args (`AppViewModel.swift:231-317`).
+- **Seams**: `EventKitStoring.swift:8` / `InMemoryEventStore.swift:13`;
+  `--seed` / `--ui-testing` (`AppViewModel.swift:246-317`);
+  `AppGroup.defaults` (`AppGroup.swift:27-30`, single cached instance);
+  preview/test `ReminderStore` injection.
+- **Framework split**: `SingleThreadTests` and `SingleThreadWatchTests` are
+  Swift Testing (`import Testing`, `@Test`); `SingleThreadUITests` and
+  `SingleThreadWatchUITests` are XCTest. `SingleThreadTests` is the single
+  combined iOS+macOS unit suite; platform selection is by whole-file/inline
+  `#if os(...)` (`AppDelegateTests.swift:1`, `SettingsViewTests.swift:82`).
+
+**Coverage reality** (research Q1/Q2 plus a targeted analyzer pass):
+
+- Core has ~45 source files and ~54 unit-test files; most types have a
+  matching `*Tests.swift`. The research "gap trio" is a false alarm:
+  `ReminderDateFilter.swift:28` is covered by
+  `SingleThreadTests.swift:173-248`, `ReminderIntentSupport.swift:30` by
+  `ReminderIntentSupportTests.swift`, and `EntitlementState.swift:10` by
+  `EntitlementSyncTests.swift:113-123`.
+- **Widget** (`NextThingWidget.swift`, `SingleThreadWidgetBundle.swift`) is
+  the only module with zero unit tests.
+- **Watch** `ShowAlarmsState.swift:13`, `ShowDateState.swift:13`,
+  `ShowListState.swift:13`, `ShowRecurrenceState.swift` are untested,
+  structurally identical to the tested `ShowCompletionGlowState` /
+  `ShowEnableActionButtonsState` holders, and reachable from
+  `SingleThreadWatchTests` via `@testable import SingleThreadWatch`
+  (`ShowCompletionGlowStateTests.swift:3`) with no pbxproj change.
+- **`FoundationModelsReminderRankerTests.swift`** only asserts
+  `isAvailable` self-consistency; real ranking is never invoked (the model
+  is unavailable on CI, so it cannot be invoked deterministically).
+- Remaining untested iOS files cluster in SwiftUI `View`/modifier bodies
+  (`EmptyStateCard`, `ReminderCardView`, `ControlPlateModifier`, …) where a
+  unit test asserts nothing real.
+
+## Desired End State
+
+A bounded, high-signal test-suite audit that:
+
+1. Closes the concrete coverage gaps where pure/stateful logic actually
+   exists — watch `Show*State` holders, the widget's extracted logic, and
+   the AI-sort coordinator's trust/fallback seam.
+2. Extracts the widget's testable logic into `SingleThreadCore` so it is
+   testable without standing up an app-extension test target.
+3. Ships a written audit report enumerating remaining gaps and risks as
+   explicit non-goals/follow-ups.
+4. Does **not** churn already-well-seamed source.
+
+**Verification**: new suites are green under targeted `-only-testing:` runs
+(`SingleThreadTests`, `SingleThreadWatchTests`), `make format` + `make lint`
+clean, and the full CI-identical gate (`scripts/test.sh`, via the `run-gate`
+skill) passes once after phases commit.
+
+## Patterns to Follow
+
+Follow (existing, good):
+
+- **Core-pure-logic tests**: `ReminderSkipTests.swift`,
+  `ReminderSortTests` (struct in `ReminderSkipTests.swift`),
+  `ReminderDateFilterTests` in `SingleThreadTests.swift:173-248` —
+  deterministic, no EventKit.
+- **`InMemoryEventStore` injection**: `ReminderStoreTests.swift` (90+
+  injections, `InMemoryEventStore()` + `loadsReminders: false` + pre-seeded
+  `reminders/skippedIDs`); private fakes like
+  `CompletedReturningEventStore` at `ReminderStoreTests.swift:1086`.
+- **Serialized Swift Testing for real-`UserDefaults` holders**:
+  `ShowEnableActionButtonsStateTests.swift:8` and
+  `ShowCompletionGlowStateTests.swift:12` both use `@Suite(.serialized)`
+  because every test writes the same real key. New watch `Show*State`
+  suites must do the same.
+- **Protocol-seam fakes**: `FakeSession`
+  (`SingleThreadTests/TestFixtures.swift:70`, os-gated), the `ReminderRanking`
+  protocol (`AIReminderRanking.swift:36`) for a fake ranker.
+- **Naming**: unit-test names must NOT start with `test`/`testing`
+  (SwiftFormat strips the prefix); UI-test names keep `test…`.
+
+Do NOT follow:
+
+- Whole-file UI/XCTest suites as a model for new tests — unit tests are
+  Swift Testing and belong in the unit targets.
+- Adding a test target "because a module has none" — the widget's pure
+  surface is too thin to justify the app-extension test-target cost
+  (pbxproj IDs, scheme TestAction, `scripts/test.sh`, Makefile, CI matrix,
+  plus an unproven `@testable import` of an app-extension product).
+- Asserting only self-consistency (`FoundationModelsReminderRankerTests`'s
+  `isAvailable` pattern) — every new test must assert behavior.
+
+## Design Decisions
+
+1. **Value bar — logic-first.** New tests target pure functions, state
+   machines, persistence round-trips and view-model branch logic. SwiftUI
+   `View` bodies and trivial modifiers are skipped and named in the report
+   with a one-line rationale. Coverage numbers may *inform* prioritization
+   but no line-coverage threshold gates the work.
+
+2. **Widget — extract to Core.** Move the widget's testable logic
+   (refresh-cadence/timeline-date math and entry construction from
+   preference/authorization state) into a pure `SingleThreadCore` type that
+   both the widget (already a `SingleThreadCore` dependency, pbxproj:357)
+   and `SingleThreadTests` import. The SwiftUI `NextThingWidgetView` stays
+   untested. No new target.
+
+3. **Watch `Show*State` — test, then flag.** Add dedicated, `@Suite(.serialized)`
+   Swift Testing files under `SingleThreadWatchTests/` mirroring
+   `ShowEnableActionButtonsStateTests` (init-from-pref fallback, `apply`
+   round-trip, persistence). Flag the `.standard` vs. `AppGroup.defaults`
+   divergence from `ShowEnableActionButtonsState` as a finding for user
+   triage; do NOT change persistence in this ticket (these look watch-local
+   display prefs, not phone-shared values).
+
+4. **Refactors — targeted only.** Source changes are limited to what a test
+   requires: widget logic extraction (decision 2) and, if needed, an
+   injection seam for the ranker. No broad View/ViewModel extraction; the
+   `EventKitStoring` + injectable-`ReminderStore` seams already cover the
+   testability surface.
+
+5. **Bounding — phases + a written report.** Work is implemented in
+   module-scoped phases (watch `Show*State`; widget extraction + Core tests;
+   AI-sort coordinator trust/fallback via a fake `ReminderRanking`), each
+   verified with targeted suites. The audit's output is a report artifact
+   listing remaining gaps and risks. `FoundationModelsReminderRanker`'s
+   real-model behavior is only ever exercised through a fake ranker at the
+   protocol/coordinator boundary — never the live model.
+
+## What We're NOT Doing
+
+- No new test target (widget or otherwise) and no pbxproj/scheme/CI-matrix
+  wiring.
+- No tests for SwiftUI `View`/modifier bodies or for the widget's SwiftUI
+  view.
+- No attempt to invoke `FoundationModels` in tests; no CI-dependent AI
+  behavior assertions.
+- No persistence-semantics changes (AppGroup vs. `.standard`) — the
+  divergence is reported, not fixed.
+- No coverage-threshold gate; no exhaustive file-parity coverage.
+- No child tickets; all work lands on the main ticket branch.
+- No changes to already-covered Core "gap trio".
+
+## Open Risks
+
+- **Widget extraction scope**: `makeEntry` (`NextThingWidget.swift:61-96`)
+  is `@MainActor` and reads preference stores + `EKEventStore` authorization
+  status. Extracting only the pure parts may leave a thin, low-value surface;
+  the plan must confirm the extracted type is meaningfully testable before
+  committing to it. If not, widget logic becomes a documented non-goal.
+- **Watch persistence divergence**: whether `showDate` / `showList` /
+  `showRecurrence` / `showAlarms` are watch-local or phone-shared was not
+  confirmed; if they are phone-shared, decision 3's "flag, don't fix" leaves
+  a real bug open (reported, not silently deferred).
+- **Ranker seam**: if `AISortCoordinator`'s trust/fallback paths are already
+  fully covered by `AISortCoordinatorTests.swift` (23 matches), the
+  fake-ranker work may reduce to a small number of added cases; the plan
+  must diff existing coverage before writing new tests.
+- **macOS unit phase**: `SingleThreadTests` runs on macOS too, so any new
+  iOS-only test must declare `#if os(iOS)` (or be macOS-safe); three known
+  local-only macOS `EntitlementStoreTests` failures are pre-existing and must
+  not be debugged.

.pi/orksorksorks/alanvardy-var-1075-test-suite-audit/done.md

@@ -0,0 +1,46 @@
+# Done
+
+- **Branch / head SHA**: `alanvardy-var-1075-test-suite-audit` at `03c59904`
+  (full CI-identical gate confirmed at `9219a69a`; `03c59904` is the
+  docs-only report-status follow-up).
+- **Mechanical checks**:
+  - `make format` clean, `make lint` → 0 violations / 0 serious across 214 files.
+  - Full `./scripts/test.sh` gate **PASS** at `9219a69a` via the `run-gate`
+    async managed worktree (SIM pinned by UDID `D8020BD8-…`, watch 26.5 UDID
+    unpaired): deployment-target checks, format, `swiftlint --strict`, warning
+    self-test (20 fixtures), iOS + watch builds, Periphery (`No unused code
+    detected`, zero findings), iOS UI tests, watch UI tests, watch unit tests,
+    macOS unit tests, warning epilogue → `✅ All CI checks passed.`
+  - Targeted macOS `EntitlementStoreTests` pre-check: 8/8 passed on this dirty
+    host; the canary recorded an *expected failure* (`withKnownIssue`) as
+    designed.
+  - Gate worktree torn down and its `pi-subagents/gate-576dd88-…` branch
+    deleted (reachable from the pushed ticket branch).
+- **Review outcome** (one bounded `reviewer`, fresh context, report citations
+  cross-checked by hand):
+  - **Blocker B1 fixed** — `audit-report.md` §6 was stale after commit
+    `4180ee4d`: rewritten as "Host StoreKit store — dirty-host accommodation"
+    with corrected lines (`:43`, `:83`, `:108`, `:129`) and the non-failing
+    canary semantics.
+  - **Fix applied** — `hostStoreKitIsClean()` is no longer a discarded read: it
+    reports a dirty host as a `withKnownIssue` carrying the old actionable
+    "Clear via Xcode → Debug → StoreKit → Manage Transactions…" message, so
+    the signal is visible while CI can never be turned red by host state
+    (your explicit requirement). Verified live on this machine.
+  - **Reviewer false positive rejected** — the claimed §2a/§4 citation drift
+    (`ShowDateState`, `ShowEnableActionButtonsState`, `AppGroup.defaults`) was
+    checked against source and is correct as written; no edit made.
+  - **Declined / deferred** — N1 (no-op `waitUntil { emitted.count == 1 }`)
+    and the F2 guard comment were not applied; both are cosmetic.
+  - Report status updated to "Confirmed — full gate passed at `9219a69a`".
+- **Remaining manual items** (for you, not blocking merge):
+  - Build + run the iOS app, add the "Next Thing" widget to a simulator home
+    screen, confirm it renders a reminder or the no-access message (behavior
+    unchanged by the Phase 2 extraction).
+  - Optionally comment out one preference read in
+    `NextThingDisplayPreferences` locally and confirm a unit case fails, to
+    prove the tests bind to the extraction (red-first check).
+  - **User triage decision** on audit-report §4: the five watch `Show*State`
+    holders persist to `UserDefaults.standard` while
+    `ShowEnableActionButtonsState` uses `AppGroup.defaults` — intentional
+    (watch-local) or a latent bug? Left recorded, not fixed.