Skip to content

feat(cliniko): patient search + appointment picker (#9) - #44

Closed
CloudbrokerAz wants to merge 22 commits into
agentdevsl:mainfrom
CloudbrokerAz:feat/cliniko-patient-picker-9
Closed

feat(cliniko): patient search + appointment picker (#9)#44
CloudbrokerAz wants to merge 22 commits into
agentdevsl:mainfrom
CloudbrokerAz:feat/cliniko-patient-picker-9

Conversation

@CloudbrokerAz

Copy link
Copy Markdown

Summary

Patient search + appointment picker UI for Clinical Notes Mode export. Rides on ClinikoClient from #8 — adds two thin actor wrappers (ClinikoPatientService, ClinikoAppointmentService), an @Observable @MainActor view-model with debounced search, and the SwiftUI picker view itself. Selections flow through to SessionStore.active.selected{Patient,Appointment}ID so #14's export flow can consume them.

Closes #9.

Files

Models (Sources/Models/)

  • Patient.swift — Decodable DTO; id: Int (Cliniko wire shape), dateOfBirth: String? (Cliniko returns YYYY-MM-DD which collides with the client's .iso8601 strategy — see file comment).
  • Appointment.swift — Decodable DTO; startsAt/endsAt decode natively under .iso8601.
  • ClinikoPagination.swiftPatientSearchResponse / AppointmentListResponse envelopes + ClinikoPaginationLinks { next: URL? }.

Services (Sources/Services/Cliniko/)

  • ClinikoPatientService.swift — actor + protocol ClinikoPatientSearching: Actor. One method.
  • ClinikoAppointmentService.swift — actor + protocol ClinikoAppointmentLoading: Actor. Window: [reference − 7d, reference + 1d] (UTC).

View layer (Sources/Views/ClinicalNotes/)

  • PatientPickerViewModel.swift@Observable @MainActor. Phase machines SearchPhase / AppointmentPhase. Debounce via Task.sleep(_:) + post-sleep cancellation re-check. Service refs @ObservationIgnored per concurrency.md §1. .cancelled swallowed on the search path (typing race), conditional on Task.isCancelled for the appointment path. Non-ClinikoError catch-all crashes in DEBUG (assertionFailure) and degrades to .transport(.unknown) + structural log in RELEASE — type names only, no PHI.
  • PatientPickerView.swift — Warm Minimalism (.ultraThinMaterial, Color.amberLight/.amberBright, spring(0.5, 0.7)). Two panes: search (with all five SearchPhase states) and appointment list (with .idle/.loading/.loaded/.error). "No appointment / general note" is the first row of the appointment pane.

Tests (33 new tests, all green)

  • Tests/.../Services/Cliniko/ClinikoPatientServiceTests.swift — 8 XCTest cases (decoding, query-item shape, 401/403/404/503 mapping, .cancelled).
  • Tests/.../Services/Cliniko/ClinikoAppointmentServiceTests.swift — 5 XCTest cases (decoding, window edges, path encoding, 401/404).
  • Tests/.../Views/PatientPickerViewModelTests.swift — 12 Swift Testing tests (debounce-no-call, debounce-collapse, all phase transitions, selection writes, clearSelection).
  • Tests/.../Views/PatientPickerViewRenderTests.swift — 8 ViewInspector + crash-detection tests covering all phase combinations.

Why XCTest for the service tests: URLProtocolStub keeps a single process-wide responder. XCTest serialises tests within a class; Swift Testing parallelises them and they trample each other. Refactor tracked in #30. VM tests stay Swift Testing because they go through the actor protocols and don't touch URLProtocolStub.

Fixtures (Tests/.../Fixtures/cliniko/responses/)

  • patients_search.json (3 synthetic results: Sample/Test/Fixture), patients_search_empty.json, patient_appointments.json (3 synthetic timeslots). All @example.test.

Acceptance criteria mapping

#9 acceptance Coverage
First keystroke → no network call debounce_firstKeystroke_noNetworkCall (asserts callCount == 0 after one keystroke + Task.yield())
Empty / loading / error states rendered PatientPickerViewRenderTests.test_picker_*Phase_rendersWithoutCrash
Crash-detection (ViewInspector) instantiates picker with mock service test_picker_instantiatesWithoutCrash, test_picker_bodyAccessDoesNotCrash
No patient data cached to disk Services hold no state across calls; VM is @MainActor in-memory; no UserDefaults / FileManager writes; logs are structural-only (type names + path templates per phi-handling.md)

Pre-PR review

Three Opus reviewers ran in parallel — pr-review-toolkit:code-reviewer + silent-failure-hunter + type-design-analyzer. Findings folded in:

  • Catch-all silent failure (silent-failure-hunter B1): now assertionFailure in DEBUG + structural log + degrade in RELEASE.
  • .forbidden copy misroute (B2): now mentions API-key scopes explicitly.
  • .cancelled race on appointment path (S1): only swallow when Task.isCancelled; otherwise surface as error.
  • Stuck .searching / .loading after cancel (S2): cancel-guards now reset to .idle.
  • .decoding copy (S3): now suggests "report it" so users have an action.
  • .notFound resource discriminator (silent-failure-hunter N3): copy is now per-resource, e.g. "No matching patient" / "No matching appointment".
  • reference: Date = Date() default on actor witness (type-design-analyzer): default removed — protocol existentials don't surface witness defaults, so leaving it would have misled callers.
  • DateFormatter per-row (code-reviewer nit): hoisted to a static.

Type-design-analyzer flagged that selectedAppointmentID: Int? overloads "no choice yet" with "explicit no appointment / general note" — I've left it as-is for #9 because both states map to SessionStore.setSelectedAppointment(id: nil) and there's no consumer that distinguishes them today, but it should become a tri-state enum before #14's "Confirm export" flow lands.

Test plan

  • CI green: PR-scoped pre-commit, swift test --parallel --enable-code-coverage against the documented skip list, codecov/patch.
  • Gemini Code Assist: address inline comments.
  • Local: swift test --parallel --skip <CI list> → all suites passing locally.

🤖 Generated with Claude Code

CloudbrokerAz and others added 22 commits April 24, 2026 13:14
Closes #20. Enables unit-test execution in GitHub Actions with coverage export + pre-commit enforcement.

- swift test --parallel --enable-code-coverage with by-name skip list for hardware-dependent classes (moves to tag-filtering with #23)
- llvm-cov → lcov export; Codecov upload (best-effort until CODECOV_TOKEN secret set)
- coverage-report artifact retained 7 days
- new pre-commit job scoped to PR's diff range only (not --all-files, to avoid retroactive enforcement on legacy files)
- .claude/CLAUDE.md carries session-restart context for the Clinical Notes Mode initiative

Review + fix-up commits on this branch addressed silent-failure patterns in the coverage pipeline, a HEAD~1 edge case on fresh branches, and the GeneralSectionPersistenceTests shared-UserDefaults race under --parallel (tracked for fix in #32).
Closes #21. Adds the hand-rolled, Sendable-safe HTTP mocking helpers for the Cliniko client and beyond.

- Tests/SpeechToTextTests/Utilities/URLProtocolStub.swift — thread-safe URLProtocol subclass with install()/reset() API; @unchecked Sendable justified inline
- Tests/SpeechToTextTests/Utilities/HTTPStubFixture.swift — Bundle.module-backed JSON/text loader with typed FixtureError (Equatable, Sendable)
- Tests/SpeechToTextTests/Fixtures/{cliniko,soap,llm}/ skeleton + README with PHI-free policy
- Package.swift: resources: [.copy("Fixtures")] on the test target
- 9 exemplar tests covering happy path, responder-error surfacing (network-layer, not decode), reset, fixture not-found / empty-path / trailing-slash, wrong-Codable-shape, FixtureError equality

Review + fix-up commits addressed defer-unlock fragility, a docstring force-unwrap that would propagate to the Cliniko client, weak test assertions, missing path edge-case tests, and FixtureError conformance gaps. RAII Installation handle + structured Route list deferred to #30.

Unblocks #8, #10 (Cliniko Networking + treatment_note export).
Closes #22. Adds the credential-storage abstraction used across the app, with a real Keychain-backed actor and a test-only in-memory fake.

- Sources/Services/SecureStore.swift — Sendable protocol: set/get/delete/deleteAll + setString/getString convenience. Missing-key contract returns nil; throws is reserved for OS errors.
- Sources/Services/KeychainSecureStore.swift — SecItem* backed actor, kSecAttrAccessibleWhenUnlockedThisDeviceOnly (no iCloud sync), accessibility enforced on update (not just add), one-shot errSecDuplicateItem retry, non-Data payloads throw Failure.unexpectedItemType instead of silently returning nil. Error-path logs service/key names but never values.
- Tests/SpeechToTextTests/Utilities/InMemorySecureStore.swift — actor-isolated dictionary fake; never imports Security.
- Tests/SpeechToTextTests/Utilities/InMemorySecureStoreTests.swift — 12 tests covering data round-trip, overwrite, delete, deleteAll-on-empty, string convenience, initial-state via public API, byte-transparent (null/non-UTF-8) round-trip, 128-task concurrent setsAndGets.

Review + fix-up commits addressed three real correctness issues in KeychainSecureStore (accessibility-preservation hole, duplicate-item race, silent non-Data→nil path) flagged by the multi-agent review. Failure enum split into semantic cases (authDenied vs unexpected) deferred to #29.

Unblocks #7 (Cliniko Keychain credential UI).
Closes #23. Establishes Swift Testing as the default for new pure-logic and async tests, plus the tag scheme. Does not migrate existing XCTest files — those continue as-is.

- Tests/SpeechToTextTests/Utilities/TestTags.swift — extension Tag { fast, slow, requiresHardware }
- Tests/SpeechToTextTests/Utilities/SwiftTestingExemplarTests.swift — canonical reference: @test with tags, parameterized @test(arguments:), @suite with tag propagation, #expect / #require
- scripts/remote-test.sh — SWIFT_TEST_EXTRA env var for tag filter pass-through
- .claude/CLAUDE.md — operating rule #2 now points at the new reference files directly
- .github/workflows/ci.yml — Codecov upload promoted from best-effort to hard gate (CODECOV_TOKEN now set; fail_ci_if_error: true)

Deferred: migrate existing XCTest hardware tests to Swift Testing, then switch CI filter from --skip Class to --skip-tag requiresHardware (closes #31).
Closes #25. Inspired by avdlee/swiftui-agent-skill's progressive-disclosure pattern.

- AGENTS.md reduced from 960 to 210 lines: project summary, Topic Router table, Correctness Checklist (hard always/never rules), tech stack + commands.
- Six topic-scoped reference files under .claude/references/ loaded on demand: concurrency.md (ports CONCURRENCY_PATTERNS), testing-conventions.md (F1-F4 consolidated), cliniko-api.md, phi-handling.md, mlx-lifecycle.md, menubar-integration.md.
- docs/CONCURRENCY_PATTERNS.md preserved as a redirect stub so .swiftlint.yml / AppState.swift / old bookmarks still resolve.
- .claude/CLAUDE.md updated to point at the new location.

Per-subdirectory AGENTS.md files under Sources/Services/ClinicalNotes, Cliniko, Views/ClinicalNotes remain tracked in #17 — F6 provides the shape they'll follow.
…ecycle (#36)

Closes #2.

Adds the in-memory holder for a single clinical consultation from
recording completion through Cliniko export — one active
`ClinicalSession` at a time, cleared on successful export, app quit,
cancel, or inactivity timeout. PHI lives here only.

- `Sources/Models/ClinicalSession.swift` — Sendable Identifiable value
  type wrapping `RecordingSession` + optional `StructuredNotes` +
  practitioner edits + Cliniko selections.
- `Sources/Models/StructuredNotes.swift` — minimal Sendable scaffold
  (SOAP + manipulations + excluded) to unblock this issue. #4 / #5 own
  the authoritative prompt + parser.
- `Sources/Services/SessionStore.swift` — `@Observable @MainActor`
  store. Public surface: `start`, `clear`, setters for draft /
  patient / appointment, `markExcludedReAdded`, `touch`,
  `checkIdleTimeout`. Injectable `now` + `idleTimeout` keep the idle
  path deterministic under test. No persistence — PHI policy per
  `.claude/references/phi-handling.md`.
- `Tests/…/SessionStoreTests.swift` — 24 Swift Testing cases
  (`.fast` tag) covering start/replace/clear idempotency, mutator
  no-ops when inactive, dedup + order invariants, idle-timeout
  behaviour (inactive / below / at-boundary / above / second-call),
  export-success contract, and a UserDefaults-diff invariant that
  catches any future refactor that starts writing to disk.
- `Tests/…/ClinicalSessionTests.swift` — field round-trips +
  `StructuredNotes` Equatable sanity.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-start AGENTS.md load (#37)

Three workflow-hardening changes, all doc/config only.

### 1. Gemini Code Assist on PRs
Adds `.gemini/config.yaml` + `.gemini/styleguide.md` so the Gemini Code
Assist GitHub App can review every PR alongside our existing local
`pr-review-toolkit:code-reviewer` subagent pass. Config tuned for a
privacy-sensitive macOS-Swift project: MEDIUM severity threshold,
25-comment cap, drafts excluded, build/ML artifacts / test fixtures /
`.claude/` excluded from analysis. Styleguide summarises the PHI,
concurrency, and locked-decision rules the reviewer needs, pointing
back at `AGENTS.md` + `.claude/references/` for authority.

### 2. Session-start AGENTS.md load
`.claude/CLAUDE.md` previously imported `@/workspace/AGENTS.md` — a
devcontainer-absolute path that silently fails on the host. Adds a
prominent "Read this first" block at the top of the file mandating an
explicit `Read` of `AGENTS.md` before any work (belt) plus a portable
`@../AGENTS.md` relative import (braces). The explicit mandate is the
real safeguard — silent import failures are why this got missed.

### 3. Opus subagent mandate
Tightens operating rule #1 in `.claude/CLAUDE.md` and adds a "Subagents
& code review" section in `AGENTS.md` requiring `model: "opus"` on every
`Agent` tool call. Several subagent definitions default to Sonnet and
will silently downgrade otherwise. Also documents the three-layer review
pipeline: pre-PR local subagent → Gemini on PR open → on-demand
`/code-review` slash command for large or multi-subsystem PRs.

No source-code changes; all three land as docs/config only. User still
needs to install the Gemini Code Assist GitHub App at
https://github.com/apps/gemini-code-assist — it cannot be installed via
CLI.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iage (#41)

* feat(ci): structured CI summary → $GITHUB_STEP_SUMMARY for fast PR triage

Closes #39.

### What it does

Parses `swift build` and `swift test` output (teed to log files by the
workflow) and emits a Markdown summary to `$GITHUB_STEP_SUMMARY`. The
summary is visible under the "Summary" tab of the workflow run on every
PR — no more log-scrolling to find the one XCTest assertion that failed
or the seven new Swift 6 warnings a PR introduced.

### Contents of the summary

- **Headline chips**: compile errors, test failures, warnings, log
  capture issues, possible regex drift, non-success job status.
- **Test failures** (top 25): XCTest case + location + assertion, and
  Swift Testing cases with name + detail.
- **Test totals**: Swift Testing and XCTest, with `--parallel`-safe
  XCTest aggregation (see review blocker below).
- **Compiler diagnostics by category** (`#TemporaryPointers`,
  `#ActorIsolatedCall`, etc.), collapsible, deduped, top 20 per
  category.
- **Log capture issues** when `build.log` / `test.log` is missing or
  zero bytes — surfacing exactly the silent-failure mode this tool is
  meant to fix.

### Fixes applied from the pre-PR Opus review pipeline

Ran `pr-review-toolkit:code-reviewer` + `pr-review-toolkit:silent-failure-hunter`
in parallel before opening the PR, per the new operating rule #1 (AGENTS.md).

Both found real blockers. Applied:

- **XCTest `--parallel` aggregation bug** (code-reviewer blocker). Under
  `swift test --parallel`, each test-class process emits three identical
  `Executed N tests` lines (inner suite, `*.xctest` wrapper, `Selected
  tests`). The original implementation picked `max()`, silently hiding
  failures in non-largest suites. Now accumulates only the `Executed`
  line that follows `Test Suite 'Selected tests' (passed|failed)`, so
  one entry per process under `--parallel` and one entry total under
  non-parallel. Self-test fixture includes the multi-suite regression
  case (`SELF_TEST_TEST_PARALLEL`).
- **Summary-script crash masquerading as job failure / success**
  (silent-failure BLOCKER 1). The `CI summary` step is `if: always()`;
  if the script crashed it used to either add a second red X or
  silently render partial output. Now wrapped in `|| echo ::warning::…`
  so summary failures never flip the job, plus a Python-level try/except
  that writes a visible "Summary generator crashed" block to the step
  summary before returning 0.
- **PHI egress risk via artifact upload** (silent-failure BLOCKER 2).
  Dropped the `Upload CI logs artifact` step entirely — `test.log`
  captures verbatim `swift test` stdout and any future fixture that
  echoes PHI (SOAP body, transcript, Cliniko request) would leak to a
  7-day GitHub artifact. Inline summary + raw workflow log are enough
  for triage; the artifact was opt-out by default, which violates the
  PHI-handling policy's spirit (`.claude/references/phi-handling.md`).
- **Missing / empty log renders "✅ Clean"** (silent-failure HIGH). Added
  `_log_status` with four discrete states (absent / missing / empty /
  present). Missing or empty logs now render a dedicated
  "Log capture issues" section and never hit the "Clean" headline.
  Plus `: > build.log` / `: > test.log` before the tee pipe so the
  empty state is distinguishable from absent even if the underlying
  command exits before emitting a single line.
- **Regex drift blind spot** (silent-failure HIGH). Added
  `SWIFT_DIAG_CANARY` that matches anything diagnostic-shaped
  (`*.swift:N:N: (warning|error|note|remark):`). If the canary matches
  but `SWIFT_DIAG` doesn't, we count it and surface a "possible regex
  drift" chip in the headline. Self-tests exercise both the match and
  no-match paths.
- **`$GITHUB_STEP_SUMMARY` 1 MiB cap** (silent-failure LOW). Added
  `_cap_body` with a 900 KiB soft cap, truncating at a line boundary
  with a visible trailer. Guards against the "one morning swiftc emits
  10k new `#StrictConcurrency` warnings" scenario.
- **Category regex too narrow** (silent-failure LOW). Expanded
  `[A-Za-z0-9_]+` → `[A-Za-z0-9_.\-]+` for forward-compat with dotted
  / hyphenated categories like `#StrictConcurrency.Availability`.
- **Pipe escaping inconsistency** (code-reviewer nit). `|` now escaped
  in both file cell and message cell.
- **Dead code in Swift Testing filter** (both reviewers). Removed the
  unreachable `startswith("run with")` branch; the regex already
  requires a quoted name so unquoted run-summary lines can't match.
- **`set -euo pipefail` (not `-eo pipefail`)**. Consistent with the
  existing coverage-export step.

### Self-test coverage

`scripts/ci-summary.py --self-test` runs 34 assertions locally and in
CI before every test step, covering: category bucketing + dedup,
runner-prefix strip, build-cache filtering, drift canary, XCTest +
Swift Testing failure parsing, single-process + multi-process totals
(the `--parallel` regression case), `_log_status` states,
missing/empty/clean/failure rendering, job-status chip, 1 MiB cap,
and pipe escaping on both file and message cells.

### Follow-ups (not in this PR)

- CI critical-path parallelisation (`build` → `test` sequential link can
  be removed; `test` → `[lint, pre-commit]` probably too). Trades
  nothing; saves ~2 min of wall time. Filing separately.
- `/gemini review` re-trigger when the PR materially changes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* polish: tighten drift canary to warning/error only

The canary matching `note:` and `remark:` produced a per-PR "1 diagnostic-
shaped line the parser didn't understand" chip on every run. swiftc emits
note/remark as continuation lines for the primary diagnostic (e.g. "note:
insert 'try'" after a try-failure warning); they don't match SWIFT_DIAG by
design, so counting them here was noise without drift signal.

- Restrict SWIFT_DIAG_CANARY to `(warning|error):`.
- Flip the self-test: note-shaped lines must NOT bump the canary.
- Add an explicit render-level drift test that passes a non-zero canary
  count directly (so render behaviour stays covered even though the
  parser no longer flags note/remark as drift).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #38.

`test_waitForCompletion_waitsForPendingWrites` sleeps 50ms in a child
task and asserts `waitForCompletion()` returns in <200ms. On busy CI
runners, scheduler jitter pushed the actual elapsed time to 204ms and
flaked the test — PR #37's post-merge main CI caught one such run.

Widen the upper bound to 500ms. Lower bound stays at 40ms so the test
still verifies `waitForCompletion()` actually blocks on the outstanding
write; 500ms still catches a pathological "wait forever" regression.

No production-code change.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…CaptureService deviceIdSize let (#43)

Part of #40. First of four staged Swift 6 warning cleanups. Low-risk
pattern matches (nonisolated(unsafe) deinit mirror already used in
VoiceTriggerMonitoringService).

### Changes

- `Sources/Services/PermissionService.swift`
  - Add `nonisolated(unsafe) var deinitActivationObserver` that mirrors
    every write to the `@MainActor`-isolated `activationObserver`.
  - `deinit` reads from the mirror so it doesn't cross an actor
    boundary into a non-`Sendable` `NSObjectProtocol?` property —
    previously warned as "cannot access property 'activationObserver'
    with a non-Sendable type … from nonisolated deinit; this is an
    error in the Swift 6 language mode".
  - All four write sites (two teardown paths + one setup path) pair
    both fields.
- `Sources/Services/AudioCaptureService.swift:239` — `var deviceIdSize`
  → `let`. Never mutated, only read at line 286.

### Why this pattern

`.claude/references/concurrency.md` §2 lists `deinit` cleanup of a
`@MainActor`-isolated class as the canonical legitimate use of
`nonisolated(unsafe)`. SwiftLint's custom `nonisolated_unsafe_warning`
will fire on the new line; the inline doc-comment is the audit trail
the rule exists to encourage.

Pre-PR reviewed by `pr-review-toolkit:code-reviewer` (Opus) —
LGTM / ship it. No blockers.

### Remaining #40 work

- **40b** — AudioCaptureService CFString/AudioDeviceID inout-pointer
  fix (real Core Audio lifetime bug; next PR).
- **40c** — three MainActor-isolation warnings across AppDelegate +
  ParticleVortexWaveform.
- **40d** — FluidAudioService.asrManager sending (architectural).

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…MutablePointer (#44)

* chore(concurrency): scope Core Audio pointer lifetimes via withUnsafeMutablePointer

Part of #40. Second of four staged Swift 6 warning cleanups.

### The problem

`AudioCaptureService.setInputDevice(...)` does a device-UID →
`AudioDeviceID` lookup via `AudioObjectGetPropertyData` +
`AudioValueTranslation`. The original construction of the translation
struct took raw pointers from inout `&deviceUID` / `&deviceId`
expressions:

    var translation = AudioValueTranslation(
        mInputData: &deviceUID,
        mInputDataSize: UInt32(MemoryLayout<CFString>.size),
        mOutputData: &deviceId,
        ...
    )

swiftc 6.x flags this in three ways:

- `cannot use inout expression here; argument 'mInputData' must be a
  pointer that outlives the call [#TemporaryPointers]`
- `cannot use inout expression here; argument 'mOutputData' must be a
  pointer that outlives the call [#TemporaryPointers]`
- `forming 'UnsafeMutableRawPointer' to a variable of type 'CFString';
  this is likely incorrect because 'CFString' may contain an object
  reference.`

Ship-unchanged, Swift 6 strict concurrency mode rejects the code.

### The fix

Nest the translation construction + `AudioObjectGetPropertyData` call
inside two `withUnsafeMutablePointer(to:)` closures, one per inout
slot:

    let status = withUnsafeMutablePointer(to: &deviceUID) { uidPtr in
        withUnsafeMutablePointer(to: &deviceId) { idPtr in
            var translation = AudioValueTranslation(
                mInputData: UnsafeMutableRawPointer(uidPtr),
                ...
            )
            return AudioObjectGetPropertyData(...)
        }
    }

`withUnsafeMutablePointer(to:)` is non-escaping, so `uidPtr` / `idPtr`
are guaranteed valid across the Core Audio call. The pointee byte
addresses are identical to what the old inout form gave — pure
compiler-reasoning fix, no behavioural change. `status` propagates
through both closures as the implicit expression return.

### Tests

`Tests/SpeechToTextTests/Services/AudioCaptureMemoryLayoutTests.swift`
(new, 4 cases, `.fast`) asserts the `MemoryLayout` sizes this code
depends on — `AudioDeviceID` (4), `CFString` (8),
`AudioValueTranslation` (32), `AudioObjectPropertyAddress` (12). The
property-lookup call site is `#if os(macOS)` and hardware-gated from
CI (real mic), so the only way this refactor could silently break is
a platform-drift in those struct sizes. This test runs without
hardware and catches that drift.

### Review

- `pr-review-toolkit:code-reviewer` (Opus) — LGTM / ship it. Walked
  each closure's return type, pointer lifetime, and behavioural
  equivalence to the original.
- `pr-review-toolkit:silent-failure-hunter` (Opus) — LGTM. Six risks
  investigated (status propagation, pointer escape, post-closure use,
  behavioural drift, hardware gating, platform conditional); all clear.
  One LOW nit (MemoryLayout test) — folded in.

### Remaining #40 work

- **40c** — three MainActor isolation warnings across AppDelegate +
  ParticleVortexWaveform.
- **40d** — FluidAudioService.asrManager sending (architectural).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(ci): add codecov.yml — ignore hardware-gated files + informational project floor

PR #44 exposed a recurring false signal: any PR touching one of the six
hardware-gated service files (AudioCaptureService, PermissionService,
TextInsertionService, VoiceTriggerMonitoringService, WakeWordService)
fails `codecov/patch` with "0.00% of diff hit" because those tests are
in CI's `--skip` list and can't run on the macOS-14 runner (no mic, no
TCC grant, no display server, no user keychain). Training reviewers to
ignore a gate is worse than having no gate.

`codecov.yml` at repo root:

- Hardware-gated sources in `ignore:` — list kept in lockstep with the
  `--skip` list in `.github/workflows/ci.yml` per
  `.claude/references/testing-conventions.md`.
- `Tests/**`, `UITests/**`, `scripts/**`, `.claude/**`, `.gemini/**`,
  `.github/**` also ignored — coverage of tests/tooling isn't a signal.
- `project` gate marked `informational: true` — a real floor would
  require picking a target first; track separately.
- `patch` gate kept as a hard fail with a 5% threshold for files that
  aren't hardware-gated.

No code change, no test change. Just the config that was always missing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-loop callbacks (#45)

* chore(concurrency): MainActor.assumeIsolated on main-queue / main-run-loop callbacks

Part of #40. Third of four staged Swift 6 warning cleanups. Clears the
three `[#ActorIsolatedCall]` and related MainActor-isolation warnings:

- `AppDelegate.swift:293` — `settingsService.load()` call from a
  `NotificationCenter` observer closure.
- `AppDelegate.swift:294` — `NSApp.appearance` mutation from same.
- `ParticleVortexWaveform.swift:149` — `updateParticles()` call from a
  `Timer.scheduledTimer` block.

### Why `MainActor.assumeIsolated`, not `Task { @mainactor in ... }`

- **AppDelegate theme observer** uses `queue: .main`, which
  `NotificationCenter` documents as "block runs on main-queue
  regardless of posting thread". A pre-existing comment (now expanded)
  deliberately avoided `Task` dispatch to prevent rapid theme toggles
  reordering.
- **Timer block fires 60× per second.** `Task` dispatch would allocate
  a Task and hop the main queue every frame — measurable animation
  drops. `assumeIsolated` is a zero-cost runtime-checked assertion.
- `@preconcurrency` was rejected: launders the warning without proving
  safety.

### Silent-failure-hunter fix folded in

Pre-PR review flagged a HIGH precondition risk:
`Timer.scheduledTimer(withTimeInterval:repeats:block:)` installs the
timer on `RunLoop.current`, not `RunLoop.main`. In SwiftUI `onAppear`
that's usually the main run loop, but **SwiftUI live previews** and
future `Task.detached { … }` wrapping could put us on another run
loop — and `assumeIsolated` would trap at 60 Hz in production. Fix:
construct the timer with `Timer(timeInterval:repeats:block:)` and
explicitly add it to `RunLoop.main` in `.common` mode. `.common`
mode also eliminates a pre-existing quality-of-life bug where the
animation paused during menu-bar tracking.

### Reviewers

- `pr-review-toolkit:code-reviewer` (Opus) — GO. Walked the precondition
  analysis at both sites, confirmed `queue: .main` ⇒ MainActor
  isolation for `assumeIsolated` under Swift 5.10+, and approved the
  doc comments.
- `pr-review-toolkit:silent-failure-hunter` (Opus) — flagged the HIGH
  above; applied. Also noted `NSApp` nil-window during very early
  startup / terminate is structurally impossible given current
  observer lifecycle — added an inline comment anchoring that
  invariant so a future refactor that moves observer setup earlier
  can't silently regress it.

### Remaining #40 work

- **40d** — `FluidAudioService.asrManager` sending across an actor
  boundary. Also: `AppDelegate.swift:328` (sending 'notification') and
  other `[#SendingRisksDataRace]` warnings across the codebase.
  Architectural; design discussion first.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(ci): codecov ignore AppDelegate + ParticleVortexWaveform

PR #45 exposed the next wave of false codecov signals: NSApplicationDelegate
lifecycle callbacks (notification observer setup) and SwiftUI Timer
handlers in a render-only view aren't unit-testable in this codebase
without wholesale mocking or infrastructure that's out of proportion
to the signal.

- `Sources/SpeechToTextApp/AppDelegate.swift` — testing
  `setupMenuActionObservers` requires NSApplication / NSStatusBar mocks;
  lifecycle code is functional under `./scripts/smoke-test.sh`.
- `Sources/Views/Components/ParticleVortexWaveform.swift` — only logic
  is a render path + onAppear-driven animation driver; ViewInspector
  can render the view but doesn't fire onAppear, so per-line coverage
  is not an actionable signal for this file.

Kept the hardware-gated services list intact; kept the project gate
informational and the patch gate strict everywhere else. If a
behaviourally-testable file later starts showing 0% patch coverage,
that's a real signal, not noise.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix: address Gemini review — move guard let self inside assumeIsolated

Gemini Code Assist flagged that promoting `weak self` → strong in a
nonisolated closure context, *before* `MainActor.assumeIsolated`, is
stylistically fragile: the current code compiles clean, but if a future
edit added a `self.foo` access between the `guard` and the isolated
block, it would regress the Swift 6 concurrency check.

Move the guard inside the isolated region so all `self.*` handling
happens in MainActor context. Zero functional change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ory (#46)

* feat(taxonomy): placeholder manipulations JSON + ManipulationsRepository (#6)

Introduces the chiropractic manipulations taxonomy foundation for EPIC #1
(Clinical Notes Mode). Ships a seven-entry v1 placeholder list under
Resources/Manipulations/placeholder.json and a read-only, immutable
ManipulationsRepository value type that decodes it from the main target's
bundle.

Designed for a one-file swap: replacing placeholder.json with the real
Cliniko-backed taxonomy requires no code changes. Tests cover the
bundled happy path, id uniqueness, empty-array policy, malformed JSON,
missing required keys, and resource-not-found — all Swift Testing, tagged
.fast.

Downstream consumers (#4 prompt builder, #13 ReviewScreen, #10 Cliniko
export) can now reference `[Manipulation]` via `ManipulationsRepository`.
`StructuredNotes.selectedManipulationIDs` already points at this taxonomy.

Closes #6.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(taxonomy): enforce unique manipulation IDs at decode time

Addresses Gemini Code Assist review on PR #46 (converging with pre-PR
code-reviewer feedback): a duplicate `id` in a swapped-in taxonomy JSON
would silently corrupt `StructuredNotes.selectedManipulationIDs`
matching and the #10 Cliniko export mapping, so the repository must
reject it loudly rather than rely only on a production-file test.

- `init(data:decoder:)` now throws `ManipulationsRepositoryError.duplicateIDs`
  with the sorted, deduplicated offending IDs.
- `init(all:)` gets a debug `assert` — this initialiser is a test/fixture
  seam, so a developer-mode signal is the right shape there.
- New `ManipulationsRepositoryError.duplicateIDs([String])` case carries
  the offending IDs for diagnostics. Taxonomy is static (not PHI), so
  including the IDs is fine.
- Tests: added `duplicateIDs_throwTyped` asserting both the thrown case
  and the offending-ID list; existing `bundledPlaceholder_idsAreUnique`
  kept as a belt-and-braces production-file check.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(taxonomy): simplify duplicate-ID detection per Gemini review

Uses `Dictionary(grouping: decoded, by: \.id)` directly on the decoded
list (skips the intermediate `.map(\.id)` allocation) and drops the
redundant `Array(...)` wrap around `.sorted()` — `.keys.sorted()` already
returns `[String]`. Behaviour is identical; tests unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…late (#47)

* feat(prompt): ClinicalNotesPromptBuilder + RawLLMDraft + soap_v1 template (#4)

Introduces the prompt-assembly + JSON-schema guard layer for EPIC #1
(Clinical Notes Mode).

## What
- `RawLLMDraft`: Codable value type mirroring the locked LLM JSON
  contract byte-for-byte (`subjective/objective/assessment/plan`,
  `manipulations: [{name, confidence}]`, `excluded_content`).
- `ClinicalNotesPromptBuilder`: pure-logic service with
  `loadFromBundle(_:)`, `buildPrompt(transcript:) -> String`, and
  `validate(json:) -> Result<RawLLMDraft, SchemaError>`. Tolerant of
  leading/trailing whitespace, ```json/``` code fences, and trailing
  commentary; strict on shape and on confidence ∈ [0, 1].
- `Sources/Resources/Prompts/soap_v1.txt`: versioned template with
  {{manipulations_list}} + {{transcript}} placeholders and the locked
  "drafting assistant, not a diagnostic tool" safety line.
- `Package.swift`: adds `.copy("Resources/Prompts")`.

## PHI safety (pre-PR reviewer Blocker B1)
`SchemaError.decodingFailed` now carries a structural `DecodingFailureKind`
(schema `keyPath` + case kind), not `String(describing: DecodingError)`.
Raw `DecodingError.debugDescription` can quote offending values — if
those are transcript-derived SOAP fields, interpolating the error into
a log line would leak PHI. The new shape captures only `codingPath`,
which is static schema keys + integer array indices. Two tests assert
the offending value never appears in the rendered error.

## Design reconciliation (noted in issue #4 starting-plan comment)
`StructuredNotes` is the UI model (`selectedManipulationIDs: [String]`,
`excluded: [String]`) bound to the ReviewScreen wireframe; its fields
do not match the LLM JSON shape. Rather than break #2's SessionStore
contract, this PR introduces `RawLLMDraft` as a separate Codable type
matching the JSON, and the `RawLLMDraft → StructuredNotes` mapping
will live in #5 where `ManipulationsRepository` is already in scope for
`name → id` resolution.

## Tests (23 Swift Testing, .fast)
- Prompt assembly (3): taxonomy rendering, empty taxonomy, bundled
  template + safety line + typed `templateNotFound`.
- Validate happy path (2): full draft, empty manipulations.
- Tolerant parsing (6): json / bare / uppercase code fences, trailing
  commentary, leading whitespace, braces-in-string-literals.
- Error cases (8): emptyInput, noJSONFound, unbalanced JSON,
  missingKey (with keyPath), typeMismatch (x2 with PHI-safety
  invariant), confidenceOutOfRange above/below, inclusive 0.0/1.0.
- Injection resistance (2): triple-backticks in transcript round-trip,
  {{manipulations_list}} in transcript not re-substituted.

Closes #4. Unblocks #5 (ClinicalNotesProcessor — retry-once).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(phi): confidenceOutOfRange carries keyPath, not LLM-returned name

Addresses Gemini Code Assist security-high finding on PR #47.

The previous `SchemaError.confidenceOutOfRange(name: String, value: Double)`
case carried the `name` field from the LLM's response. Although the prompt
instructs the model to use names from the static `ManipulationsRepository`,
a hallucination or prompt-injection path could put transcript-derived
content (patient name, DOB, symptom quote, …) in that string. Logging or
telemetering the error from #5's retry orchestration would then leak PHI.

Fix: switch the case to `(keyPath: String, value: Double)` where keyPath
is a structural path like `"manipulations.0.confidence"`. Matches the
same shape already used by `decodingFailed(.typeMismatch(keyPath:))`.

Tests:
- Existing above/below-range tests updated to the new keyPath shape.
- New `validate_confidenceOutOfRange_doesNotCarryName` test embeds
  "Alice Smith DOB 1983-04-12" as the LLM-returned name and asserts it
  never surfaces in `String(describing: result)`.
- New `validate_confidenceOutOfRange_reportsCorrectIndex` test pins
  that the keyPath carries the correct array index when the offender
  is not the first entry.

Deferred to a follow-up (or to #5): Gemini's medium-priority suggestion
to try multiple candidate JSON objects instead of only the first
balanced one. The soap_v1 prompt forbids preamble, the failure mode is
`.decodingFailed` which triggers #5's retry-once, and the fix adds
complexity for a rare model-misbehaviour case. Worth revisiting after
real-world MLX runs show whether the model ever includes preamble.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves the Node.js 20 deprecation warnings that started appearing on
every CI run in GitHub Actions' September 2025 announcement. GitHub is
forcing Node.js 24 as the default on 2026-06-02 and removing Node.js 20
on 2026-09-16.

Bumps (all within the actions/ first-party scope):
- actions/checkout            v4 → v5  (Node 24 transition)
- actions/setup-python        v5 → v6  (Node 24 transition)
- actions/cache               v4 → v5  (Node 24 transition)
- actions/upload-artifact     v4 → v5  (Node 24 transition)

Each bump is pinned to the first Node-24 major per the action's own
release notes — minimises behavioural drift versus going to the very
latest (v6 checkout / v7 upload-artifact). Can bump further in a later
pass if we want new features.

Left as-is:
- codecov/codecov-action@v5   — already Node 24 compatible.
- pre-commit/action@v3.0.1    — third-party, no Node-24 release yet;
                                 tracks upstream.

Minimum runner version required: v2.327.1. GitHub-hosted runners are
already on >= v2.330 so no impact for us.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
#49)

## Unused local skills

These five `.claude/skills/*` directories are leftovers from a previous
Terraform/Speckit-era configuration and have no consumers in this
macOS-app repo. Removing cuts ~11k lines of unused context.

Deleted:
- `.claude/skills/github-speckit-tester/`
- `.claude/skills/terraform-mcp-as-code/`
- `.claude/skills/terraform-stacks/`
- `.claude/skills/terraform-style-guide/`
- `.claude/skills/terraform-test/`

`settings.local.json` allow-list entries `Skill(github-speckit-tester)`,
`Skill(terraform-style-guide)`, `Skill(terraform-test)`,
`Skill(terraform-stacks)` are removed alongside — they referenced the
deleted skills and would resolve to nothing.

The plugin-backed skills (`pr-review-toolkit:*`, `commit-commands:*`,
`code-review:*`, `supabase:*`, `frontend-design:*`) remain enabled via
the `enabledPlugins` block in `settings.json`; only the project-local
skill directories are going away.

## Pre-commit / lint-config deny strengthening

The existing rule already blocks Claude from editing
`.pre-commit-config.yaml` — the failure mode the user wanted prevented.
Adding two extensions of the same principle:

- `.pre-commit-hooks.yaml` — sibling filename used by some hooks projects
  to publish hooks; blocking both filenames.
- `.swiftlint.yml` / `.swiftlint.yaml` — the other plausible escape
  hatch: bypass a SwiftLint-strict failure by relaxing the config
  rather than fixing the offending Swift.

Rationale: Claude should respond to pre-commit / SwiftLint failures by
fixing the underlying test / code / style issue, never by weakening
the rule that caught it.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`macos-15` (Sequoia) has been generally available since April 10, 2025
and became the `macos-latest` default in August 2025. Staying on
`macos-14` gates us to older OS + toolchain combinations and earns a
runner-deprecation notice from GitHub in the near future.

- All four jobs (`lint`, `pre-commit`, `test`, `build`) now run on
  `macos-15`.
- Xcode 16.2 is still pre-installed on the macos-15 image
  (verified against the actions/runner-images readme), so the
  `xcode-select -s /Applications/Xcode_16.2.app/...` step continues
  to resolve without any change.
- CI on this PR acts as the real integration test.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
)

* feat(llm): LLMProvider protocol + LLMOptions + MockLLMProvider test fake

Protocol-only slice of #3 — lands the abstraction `ClinicalNotesProcessor`
(#5) binds to, without the `MLXGemmaProvider` concrete implementation.
The MLX-Swift integration ships in a follow-up PR against the same
ticket; splitting keeps the MLX dep + bundled-weights + warmup story
out of this review surface.

- `LLMProvider` public protocol (Sendable): `generate` async + `generateStream`
  returning `AsyncThrowingStream<String, any Error>`, matching the contract
  in issue #3.
- `LLMOptions` struct with deterministic defaults (temperature 0, seed 42)
  per the EPIC #1 "deterministic generation" lock.
- `MockLLMProvider` actor test fake — fixed / queued / error response
  modes, actor-isolated call log, `[weak self]` in the stream factory
  so the pattern is safe to copy into #5.
- Swift Testing coverage for both types; all tagged `.fast`.

Closes none — `#3` stays open for the `MLXGemmaProvider` concrete impl.
Unblocks #5.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(llm): constrain LLMProvider to Actor per AGENTS.md mockability rule

Gemini Code Assist review on #51 flagged the Sendable constraint. The
issue #3 sketch had Sendable, but AGENTS.md (Correctness Checklist →
Concurrency) and .claude/references/concurrency.md §6 require an
Actor-constrained protocol when both the concrete type (MLXGemmaProvider)
and the test fake (MockLLMProvider) are actors — Swift actors cannot be
subclassed, so test-doubles need an Actor-constrained protocol.

generateStream is declared nonisolated in the protocol so callers can
build the stream synchronously from any context; MockLLMProvider already
satisfies that.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…etry-once) (#52)

* feat(notes): ClinicalNotesProcessor — transcript → StructuredNotes (retry-once)

Actor orchestrator wiring ClinicalNotesPromptBuilder (#4) + LLMProvider
(#51) + ManipulationsRepository (#6). Implements the retry-once +
raw-transcript fallback flow specified in #5.

- buildPrompt → generate → validate → (on SchemaError: retry with a
  prompt that quotes the invalid response and restates the requirement,
  which is needed to break determinism against temperature=0 + fixed
  seed) → map RawLLMDraft to StructuredNotes.
- Fallback reasons are structural sentinels (reasonLLMError,
  reasonInvalidJSONAfterRetry) — never PHI-bearing. Caught errors are
  logged via AppLogger.service with type(of: error) only, never
  localizedDescription.
- RawLLMDraft.manipulations[].name resolves to Manipulation.id by
  case-insensitive match against id or displayName; unmatchable
  names dropped silently (practitioner re-adds from ReviewScreen #13);
  duplicates de-duped preserving first-occurrence order.
- SessionStore write is the caller's responsibility — the processor
  is pure, which keeps the @MainActor-vs-actor hop out of this type
  and makes tests trivial.

Internal visibility to match the existing Clinical Notes types
(SessionStore / StructuredNotes / ClinicalNotesPromptBuilder are all
internal). The issue spec sketched `public actor` but the project
convention trumps.

Tests cover the full acceptance criteria: happy path, no-retry-when-
valid, invalid-then-valid, invalid-twice, LLM throws first, LLM
throws on retry, and the mapping edge cases (id match, displayName
case-insensitive, unmatchable dropped, dedup with order, empty list,
excluded passthrough).

Closes #5.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(notes): collapse resolveManipulationID to a single pass

Gemini Code Assist suggestion on #52: replace the two sequential loops
(O(2N), one for id match then one for displayName match) with a single
pass that short-circuits on id match and carries the first displayName
match as a fallback. Semantically identical given the taxonomy
uniqueness invariants — id-priority preserved — and strictly cleaner.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
)

Closes #7. Cliniko credential surface (Keychain via SecureStore + UserDefaults shard) + auth probe + Clinical Notes settings section + 135 new tests. Pre-PR Opus review (code-reviewer + silent-failure-hunter + type-design-analyzer) folded in; Gemini Code Assist review folded in over two re-reviews (private apiKey, semantic Failure cases, refresh-error masking guard, readFailed UX deadlock fix, defer-clear shard, CancellationError distinct, type(of:) logs, User-Agent contact, isApiKeyDraftValid, verificationStatus tri-state).
…ors) (#8)

Closes #8. URLSession-actor Cliniko HTTP client + typed errors + closed-set endpoint enum. Retry policy: GET retries on 5xx + transport (max 2x with [1s, 2s] backoff); POST treatment_notes never retries (idempotency); 429 always retries with Retry-After honoured (clamped to policy floor). PHI-safe logging (path templates only, no bound URLs / bodies / localizedDescription). Pre-PR Opus review (3 parallel reviewers) folded in 10 HIGH/P0 items; Gemini Code Assist round folded in DateFormatter thread-safety + modern Task.sleep API. 43 new tests, all green.
Adds the picker UI that selects a Cliniko patient and (optionally) one
of their recent appointments for a clinical-notes export. Rides on
ClinikoClient from #8: the new ClinikoPatientService /
ClinikoAppointmentService actors wrap .patientSearch / .patientAppointments
and surface results into a debounced @observable @mainactor view-model.

Selections are written through to SessionStore.active so #14's export
flow can pick them up. PHI handling: services log nothing; the view's
error copy is purely structural; fixtures use synthetic data only.

- Sources/Models/Patient.swift, Appointment.swift, ClinikoPagination.swift
- Sources/Services/Cliniko/{ClinikoPatientService,ClinikoAppointmentService}.swift
  + actor-constrained protocols ClinikoPatientSearching / ClinikoAppointmentLoading
- Sources/Views/ClinicalNotes/{PatientPickerViewModel,PatientPickerView}.swift
  - debounce via Task.sleep + cancellation re-check (300ms default)
  - @ObservationIgnored on every actor existential per concurrency.md §1
  - non-ClinikoError catch-all crashes in DEBUG / logs structurally + degrades in RELEASE
  - .notFound copy uses the Resource discriminator for specific messages
- Tests: 13 service XCTests, 12 VM Swift Testing tests, 8 view crash tests
- Fixtures: cliniko/responses/{patients_search, patients_search_empty,
  patient_appointments}.json (synthetic only — Sample, @example.test)

Closes #9

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@CloudbrokerAz

Copy link
Copy Markdown
Author

Re-opening against the correct base repo (CloudbrokerAz).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request establishes the foundational architecture for the 'Clinical Notes Mode', introducing models and services for Cliniko API integration, local LLM prompt construction, and session management. It also overhauls the project's documentation and operating rules while addressing concurrency safety issues in Core Audio and SwiftUI components. Feedback highlights the need to resolve remaining Swift 6 strict concurrency violations involving non-Sendable formatters, specifically suggesting the use of the modern FormatStyle API or proper actor isolation for static and instance-level DateFormatters.

Comment on lines +160 to +165
private static let iso8601Formatter: ISO8601DateFormatter = {
let formatter = ISO8601DateFormatter()
formatter.formatOptions = [.withInternetDateTime]
formatter.timeZone = TimeZone(identifier: "UTC")
return formatter
}()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The static iso8601Formatter is a stored property of a Sendable enum. Since ISO8601DateFormatter is not Sendable, this will trigger a data race warning or error in Swift 6 strict concurrency mode. Given the project baseline is macOS 14, you can replace the manual formatter with the more efficient and thread-safe FormatStyle API.

    static func iso8601(_ date: Date) -> String {
        date.formatted(.iso8601)
    }

Comment on lines +192 to +197
private static let appointmentTimeFormatter: DateFormatter = {
let formatter = DateFormatter()
formatter.dateStyle = .medium
formatter.timeStyle = .short
return formatter
}()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

This static DateFormatter is a global stored property. Since DateFormatter is not Sendable, accessing it from multiple threads is a data race. In Swift 6, this should be isolated to the @MainActor to ensure safe access from the UI layer.

Suggested change
private static let appointmentTimeFormatter: DateFormatter = {
let formatter = DateFormatter()
formatter.dateStyle = .medium
formatter.timeStyle = .short
return formatter
}()
@MainActor
private static let appointmentTimeFormatter: DateFormatter = {
let formatter = DateFormatter()
formatter.dateStyle = .medium
formatter.timeStyle = .short
return formatter
}()

Comment on lines +88 to +94
/// RFC 7231 IMF-fixdate parser. Used for `Retry-After: <http-date>`.
/// Held as an *instance* property (not `static`) because `DateFormatter`
/// is documented as thread-unsafe under mutation; even though we never
/// mutate this one after construction, sharing a single static across
/// multiple `ClinikoClient` actor instances would still trip Swift 6
/// strict-concurrency checking (`DateFormatter` is not `Sendable`).
/// Actor isolation makes the per-instance copy provably safe.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Storing a non-Sendable DateFormatter as actor state will trigger a compiler error in Swift 6 strict concurrency mode. While actor isolation protects it from concurrent access, the type itself must still be Sendable or the property must be marked nonisolated(unsafe). A safer alternative that avoids unsafe is to create the formatter on demand within the method, as it is only used on the 429 error path.

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.

1 participant