Skip to content

Dogfood edit signal follow-up: verdict parking, outcome-gated arming, inline quit flush (codex review of #202) - #208

Open
T0mSIlver wants to merge 1 commit into
mainfrom
dogfood-edit-signal-review-fixes
Open

Dogfood edit signal follow-up: verdict parking, outcome-gated arming, inline quit flush (codex review of #202)#208
T0mSIlver wants to merge 1 commit into
mainfrom
dogfood-edit-signal-review-fixes

Conversation

@T0mSIlver

Copy link
Copy Markdown
Owner

What

Follow-up to #202 (merged before its codex/GPT-5.6 review landed); fixes all six review findings on main. The feature is dogfood-only instrumentation, but every one of these corrupted the measurement it exists to produce.

  1. High — a superseded watch lost its verdict when its record write was still pending. Arming B replaced the single watch slot, so A's late attachRecord was rejected and A read as "never observed". Closed-but-unattached watches now park keyed by generation (cap 8, oldest evicted) until their attach arrives. The old test at the previous line 491 pinned this bug — it was rewritten to assert the survival, with a comment saying exactly that (this is correcting a bug-pinning expectation, not weakening).
  2. High — the watcher armed on .failed and .copiedToClipboard commits, so Backspace in the target app recorded erasing an insertion that never happened, and phantom cleans inflated the denominator. The commit outcome is now passed through; the watch arms only on .succeeded (the record still writes, honestly behavior-free).
  3. High — the quit flush ran inside a Task spawned at terminate — the exact non-guarantee its own comment warned about. flushForTermination() now runs inline in the main-queue notification callback (MainActor.assumeIsolated); only backend shutdown stays async. New seam (debugRegisterLifecycleObservers(on:)) lets the test post the real notification and assert the patch is on disk when the observer returns, with no await after post.
  4. Medium — clipboard payloads were sized by the placeholder, giving a 100-word paste a 2s window instead of 15s. The substituted text now feeds the window ladder for measurement only; the persisted record keeps the placeholder (asserted — clipboard content must not leak into dogfood records through this fix).
  5. Medium — the window timer promoted weak self to strong before its sleep, retaining the watcher for the whole window and defeating the isolated deinit cleanup. Promotion now happens after the sleep; a release test nils the last owner mid-window and asserts dealloc + monitor teardown.
  6. Low — ⇧⌘A was classified as select-all. .shift excluded; the two-gestures test extended.

Proof

  • Regression tests added, failing before / passing after (both runs via remote-build.sh dogfood, the instrumented lane — these suites are #if LOCALVOXTRAL_DOGFOOD and compile out of the plain test lane):
    before: Executed 59 tests, with 15 failures (0 unexpected)   # every finding reproduced
    after:  Executed 59 tests, with 0 failures (0 unexpected)
    
    Representative red lines per finding are in the commit message; e.g. finding 3: the patch must be on disk when the observer returns, finding 5: the window timer must not retain the watcher across its sleep.
  • Full non-instrumented unit suite green on the pre-rebase base: Executed 2015 tests, with 2 tests skipped and 0 failures; instrumented suite re-run green after rebasing onto current main (post-Remote listener: say WHY a connection was rejected, in the log and in Settings #200): Executed 59 tests, with 0 failures.
  • LLM lanes: DictationViewModel+Session.swift is on the filter path list, so the lane will run; the change passes a new parameter on the capture call and does not alter what reaches the model.
  • Not machine-verifiable: OS-level quit ordering on a real GUI session — the notification-path test pins the wiring; a hand-quit during a dogfood window on the next dogfood build is the remaining eyeball.

…ches, quit flush, payload window, timer retain, shift-cmd-A)

Codex (GPT-5.6) review findings on the post-commit edit watch, each verified
against the code and fixed test-first (red on ab0fc83, green after; both runs
via remote-build.sh dogfood — the suites are compile-gated, so the plain test
verb would silently run none of them).

1. HIGH: supersede-before-attach lost the old watch's verdict. Arming
   dictation B while A's record write was in flight replaced the single watch
   slot; A's late attachRecord was token-rejected and A's record stayed
   indistinguishable from "never observed". Closed-but-unattached watches are
   now PARKED keyed by generation (bounded at 8; oldest evicted) until their
   attach arrives. The old regression test pinned the loss ("nothing is
   written for it") and was corrected, not weakened: its contamination
   assertion (B's gesture must not reach A's record) is retained and now
   strictly stronger — A's record holds a verdict B's close must not rewrite.
   red: DogfoodEditSignalTests.swift:514 ("nil") != ("Optional(.superseded)")

2. HIGH: the watcher armed on .failed and .copiedToClipboard commits,
   recording keys against an insertion that never happened and padding the
   clean denominator. writeDogfoodCaptureIfArmed now receives the overlay
   commit outcome and arms only on .succeeded; the record is still written.
   red: DogfoodCaptureWiringTests.swift:373 startCount ("1") != ("0")

3. HIGH: the willTerminate flush ran inside a Task spawned by the observer —
   the exact pattern its own comment says is not guaranteed to run at quit.
   The flush now runs inline in the main-queue observer body
   (MainActor.assumeIsolated); backend shutdown stays in the Task.
   registerLifecycleObservers is parameterized over a NotificationCenter with
   a #if DEBUG seam so the test posts the real notification privately.
   red: DogfoodCaptureWiringTests.swift:460 behavior nil when post returned

4. MEDIUM: clipboard-payload commits measured the watch window on the
   placeholder (2 s for a 100-word paste). The session now passes the
   payload-substituted commit copy for measurement only; the record keeps the
   placeholder-bearing text and bucketed counts — the payload never persists.
   red: DogfoodCaptureWiringTests.swift:421 ("[2.0 seconds]") != ("[15.0 seconds]")

5. MEDIUM: the window timer promoted weak self to strong BEFORE the injected
   sleep, retaining the watcher for the whole window and defeating the
   isolated-deinit cleanup. sleepFor is captured by value; self is promoted
   only after the sleep resumes.
   red: DogfoodEditSignalTests.swift:657 watcher still alive mid-window

6. LOW: shift-cmd-A was classified as select-all; the predicate now excludes
   .shift alongside .option/.control.
   red: DogfoodEditSignalTests.swift:207 XCTAssertNil failed: "selectAll"

Proof: instrumented suite pre-fix: 15 failures / 0 unexpected (all six
findings reproduced). Post-fix: 59 tests, 0 failures (DogfoodEditSignalTests
22, DogfoodCaptureWiringTests 23, store 11, redaction 3). Full
non-instrumented build + unit suite: 2015 tests, 0 failures, 2 pre-existing
env-gated skips (LLMPolishModelProbeTests).
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