Skip to content

[bug-hunter] Make speaker profile merges atomic#1546

Closed
r3dbars wants to merge 4 commits into
mainfrom
codex/atomic-speaker-merge
Closed

[bug-hunter] Make speaker profile merges atomic#1546
r3dbars wants to merge 4 commits into
mainfrom
codex/atomic-speaker-merge

Conversation

@r3dbars

@r3dbars r3dbars commented Jul 17, 2026

Copy link
Copy Markdown
Owner

Summary

  • move name transfer, target update, provenance, exemplar cleanup, and source deletion into one throwing SQLite transaction
  • propagate prepare, step, row-count, begin, and commit failures to callers
  • stop post-merge cleanup when Settings observes a failed merge
  • add deterministic rollback coverage for transaction and statement failures

Proof

  • bash build-deps.sh --force
  • bash build.sh --no-open
  • bash run-integration-smoke.sh
  • focused merger tests (13 passed)
  • speaker tests (254 passed, 12 external-fixture skips)
  • full swift test (768 passed, 13 external-fixture skips)
  • bash run-tests.sh (13,189 passed)
  • git diff --check

Manual boundary

The Settings speaker-merge flow was not exercised manually.

@r3dbars

r3dbars commented Jul 17, 2026

Copy link
Copy Markdown
Owner Author

Repair pushed at 25789b6d0bcd6be53a6c9a639ff5ed6cd3405c89.

Review fixes:

  • database merge now commits before clip promotion/deletion, so a rollback leaves both clip sets untouched
  • missing/incompatible profiles now throw instead of being treated as a successful no-op
  • naming updates preserve mutation order inside one database transaction and restore the transcript if persistence fails
  • duplicate and same-name sweeps continue safely after a failed pair

Proof:

  • TRANSCRIPTED_DISABLE_FILE_LOGGER=1 bash run-tests.sh: 13,192/13,192 passed
  • focused side-effect regression: 2/2 passed
  • build source-list validation, shell syntax, and git diff --check: passed
  • final codex review --base origin/main: no actionable correctness issues
  • independent risk review: /Users/redbars/.codex/maestro/runs/20260717T114927Z-pr-1546-review-claude
  • targeted Swift package tests are locally blocked in this fresh worktree by the pre-existing missing FluidAudio dependency; GitHub CI is the package/build authority for this new head

Keeping this PR draft because the Settings speaker-merge UI/manual gate is still unproven.

@r3dbars

r3dbars commented Jul 17, 2026

Copy link
Copy Markdown
Owner Author

Fresh SwiftPM CI failure repaired at 658626ab.

What failed:

  • spm-tests could not compile SpeakerProfileMerger.swift because the new local closure referenced instance members without an explicit self capture.
  • A fresh local package compile then exposed the new rollback test comparing SpeakerProfile values even though the type is not Equatable.

Fix:

  • Added the behavior-neutral [self] closure capture.
  • Made the rollback assertion compare every persisted SpeakerProfile field explicitly, without widening the production type API.

Proof on exact head before push:

  • bash build-deps.sh --force — passed; full dependency graph and TranscriptedCore compiled.
  • swift test --filter SpeakerProfileMergerTests — 16/16 passed.
  • bash run-integration-smoke.sh — core smoke + wake smoke passed; recovery merge package tests 7/7 passed.
  • bash run-tests.sh --filter SpeakerProfileMergeSideEffectCoordinator — 2/2 passed.
  • git diff --check — passed.
  • codex review --uncommitted — no actionable findings; independent Claude diff review also returned no findings (MAESTRO_PROOF=/Users/redbars/.codex/maestro/runs/20260717T121625Z-pr1546-review-claude).

Keeping this PR draft: the Settings speaker-merge UI path still needs the stated manual proof before it is merge-ready.

@r3dbars

r3dbars commented Jul 18, 2026

Copy link
Copy Markdown
Owner Author

Disposition: superseded by current-main speaker merge replacement #1571, merged as e1f7440. The original branch was based on stale main and is not being merged.

@r3dbars r3dbars closed this Jul 18, 2026
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