After the architecture review: what keeps the code from a higher mark #73

Where the code stands after the architecture review (#70) and the comment pass (#71), and what keeps it from a higher mark. Written so each point can be picked up on its own.

What happened

#70 turned one architecture review into code: about forty bugs, each with a regression test, and the structural work in the same PR. The clipboard output became ClipboardKeeper, ClipboardSnapshot and KeyPoster; AppModel became a composition root with HotkeyRouter, PermissionMonitor, PolishModelController, CorrectionInbox and SettingsLocation beside it; the coordinator takes an injected clock and only DictationSettings; the settings file is decoded key by key; decisions became pure Core values (HotkeyWarning, HotkeySource, the timing line) that the app only words.

#71 removed and relocated comments: 4,300 comment lines in Sources/ became 1,071, the explanations went to docs/ARCHITECTURE.md with one-line pointers, a few names changed. The code is otherwise identical to #70, verified by a token-level check.

Two review rounds followed before the merge. Round one found structural problems: a stale utterance could reach the next recording, a cancel's cleanup did not wait for the previous one, a tri-state restart sentinel, the processor pipeline rebuilt on every settings change, a 1.5 GB hash that ignored cancellation. Round two found edge cases: a polish timeout that cascaded, a double monitor restart on a permission flip, a narrow clipboard restore race, a settings save that lost a row when dictionary rows had no id, a menu copy without the nspasteboard.org markers, a checksum read error reported as corruption, a raw last transcript after a failed paste, a 200 ms poll. The findings got smaller and more peripheral each round.

Rough mark for structure and testability: about 5 of 10 before, about 8 of 10 now. The caveat: the reviewer read the files the findings pointed to and the two review reports, roughly a sixth of the diff directly.

What keeps it from higher

1. The app target has no tests

Sources/Pladder is 5,098 lines with no test target. Two of round two's bugs lived there: HotkeyRouter.update restarting the monitor twice on a grant flip, and AppModel.copyLastTranscript writing NSPasteboard.general directly. Their regression tests cover only the Core and System half; the router's test passes on the old code too.

Next step, one of:

• A PladderTests target for the executable (SwiftPM can test executable targets), starting with HotkeyRouter.update against a fake coordinator seam and AppModel.copyLastTranscript against the output seam.
• Or move the remaining decisions out, the way HotkeySource.choose already is: what the router tells the coordinator on a flip, and what the inbox waits for, as Core values.

2. The coordinator is still the largest file, and where the races are

DictationCoordinator.swift is 676 lines (936 before the comment pass), and it grew during the fixes. Both review rounds found their races in its cancel and finish paths: the stale utterance, the chained cancel, the press during a cleanup, the last transcript. It rightly owns the state machine, but it also owns the warm and live loops, the finish pipeline and the shutdown.

Next step: extract the finish pipeline (engine, processors, polish, insert, timing) into a function that returns a value, and the warm and live loops into their own type driven by the same clock. Keep the state machine where it is. Benchmark before and after, as the critical-path rule says.

3. A wide concurrency surface, guarded the same way in many places

There are 12 Task.detached in Sources/, and about eight places discard a stale completion with a generation, an ID or an identity check: UtteranceSlot, ObservationLoop, CarbonHotkeySession, ClipboardKeeper's restore ID, ModelFiles.isCurrent, PasteboardOutput's owed Return, HotkeyMonitorLifecycle, OutputMuteController's recording number. The pattern is consistent and each is tested, but every one is a race site and each review round found one more (the restore task identity, the drain that ignored cancellation).

Next steps:

• Name the pattern once in docs/ARCHITECTURE.md and list the sites, so a new async piece starts from it.
• A test convention: every detached task has a test that supersedes or cancels it mid-flight with ManualClock.
• Consider one small Generation helper so the check reads the same everywhere.

4. Twenty real-time sleeps remain in tests

By file: DictationCoordinatorTests 6, S1MiniTests 2, ModelFilesTests 2, OrderedTasksTests 2, EngineLoaderTests 2, one each in PasteFlowTests, ObservationLoopTests, TranscriptPolisherBudgetTests, OutputMuteControllerTests, TestSupport, ManualClock. Some are legitimate (the audio-thread race, the cancel poll in the checksum test, the settle() helper). Each is a chance of a slow or flaky run on CI.

Next step: go through them; where a sleep stands in for ordering, replace it with the shared Gate or ManualClock; keep and label the ones that must be real time.

5. The release-to-paste path is measured only in parts

The CLI benchmarks time the processors and the engine. The coordinator's own share (capture stop hand-over, endRecording, the synchronous pipeline, the insert call) is only visible in the app's release-to-paste log line, and the before/after comparison in the running app with real dictation has not been done for #70. During round two's benchmarking the Neural Engine on the M1 failed to compile the encoder on the first run of each build (ANECCompile() FAILED (11)) and both builds measured about 0.45 s slower than the baseline in docs/BENCHMARKS.md, so that baseline needs a re-run on a good day.

Next steps:

• Dictate a few times on the merged build and compare the release-to-paste lines with a pre-review log: /usr/bin/log show --last 1h --style compact --predicate 'subsystem == "de.dinooo13.pladder"'.
• A coordinator-level timing harness in pladder-cli with a fake engine of fixed latency, so the coordinator's share has a number of its own.
• Re-run the paced baseline and update docs/BENCHMARKS.md.

6. Docs are now the source of truth for numbers the code no longer comments

docs/ARCHITECTURE.md carries the timings, limits and rules that used to sit beside the code, with 28 // See docs/ARCHITECTURE.md, "<section>" pointers. Round two found three doc/code mismatches right after the move (the custom-word three-character rule, the warm loop for a batch engine, the rewrite allowance), and one CLAUDE.md line that contradicted the Modules table.

Next step: a CI script that fails when a pointer names a heading that does not exist, and a short list in docs/ARCHITECTURE.md of which constants each number comes from, so a change to a constant has one place to update.

7. One behaviour change to confirm

Since the last-transcript fix, a dictation the processors reduce to nothing (fillers only, say) leaves the previous "Last: …" menu line instead of showing the raw text. Decide whether that is wanted; if not, the raw assignment before the processors can come back for that case only.

8. PR size

#70 was 147 files, +11,652 / −5,797. It could only be reviewed commit by commit, and two review rounds were needed to settle it. Future structural work would be easier to judge as one bug class or one split per PR, with the benchmark rule applied per PR.