Reject ambiguous overlapping repair placements

[SuhasSrinivasan]

Fixes #679.

Summary

• Detects overlapping as well as non-overlapping occurrences of an acceptor sequence within its donor sequence.
• Rejects an ambiguous repair record instead of silently projecting MM/ML calls from an arbitrary placement.
• Adds forward, reverse, repeated, unique, absent, and one-sided empty-sequence controls.

Severity

Severity: Medium — scientific correctness

Rationale: For repetitive donor sequence, the current command can report successful repair and emit plausible MM/ML tags using the first of multiple biologically inequivalent placements. This can silently move modification probabilities to incorrect acceptor positions. Exposure is data-dependent and record-local.

Root cause

str::match_indices reports non-overlapping occurrences. For donor ACACAC and acceptor ACAC, it reports the occurrence at offset 0 but skips the overlapping occurrence at offset 2, so the code incorrectly treats the placement as unique.

Implementation

The repair path uses the existing direct memchr dependency:

1. Find the first occurrence with memmem::Finder.
2. Search again from one byte after the first start.
3. Reject the record if any second occurrence exists, including an overlapping occurrence.

This uses two bounded linear, allocation-free searches and leaves the existing unique-placement projection unchanged.

Preserved behavior

• Unique forward and reverse placements retain the same MM/ML/MN projection.
• Absent placements, one-sided empty sequences, and separate non-overlapping repeats remain bounded per-record rejections.
• Per-record repair failures remain nonfatal to the command.

Non-goals

• No heuristic or alignment-based choice among ambiguous placements.
• No query-name merge, worker ordering, fatal I/O, or atomic-output change; those are addressed separately by repair can reorder or omit records and hide fatal BAM failures #680.
• No duplicate-flagged donor policy change.

Behavior before and after

Case	Before	After	Expected oracle
Donor ACACAC, acceptor ACAC	Reports one repaired record and chooses offset 0	Rejects the ambiguous record	Occurrences at offsets 0 and 2 imply no unique projection
Separate non-overlapping repeated occurrences	Rejected	Rejected	Existing ambiguity behavior preserved
Unique donor TACACG, acceptor ACAC	Emits repaired MM/ML/MN tags	Same tags	MM:Z:A+a.,0;, ML:B:C,200, MN:i:4
Reverse-strand unique and overlapping controls	Unique case repaired; overlapping case could be missed	Unique case unchanged; overlapping case rejected	Forward-sequence placement semantics preserved

Testing

Test environment

• Revision tested: 2c4a880c7087302eed279f7ddca441e4d31253a8
• Tree tested and worktree state: 9085e47e35517571724ae11ff1d2bd090b342fbf; clean after testing
• Toolchain: rustc/cargo 1.90.0
• Platform: macOS 26.6, arm64 Apple Silicon
• Reference binary: installed modkit 0.6.4
• External tools: samtools 1.23.1 for the public inline SAM/BAM reproduction
• Dependency resolution identity: ignored worktree Cargo.lock SHA-256 49c08c4c51b6f4320726551146d971fa9ef2183d40c3f6c631b2005965e242c0; memchr 2.8.3, rust-htslib 0.46.0, hts-sys 2.2.0

Test layer	Exact command, fixture, or matrix	Result and evidence
Core: parent-red regression	Exact parent 5cecc3fb3a9336068d9e3c68d5c08d678153dd2c; replay the focused forward/reverse overlapping fixtures	Parent accepts the forward ACACAC/ACAC placement and the equivalent reverse overlap instead of rejecting them
Core: focused regression	TMPDIR=/private/tmp/modkit-u58-test-tmp cargo test -p mod_kit repair_tags::tests -- --nocapture --test-threads=1	4 passed, 0 failed; overlap/non-overlap, unique/absent, reverse, and empty controls passed
Core: affected CLI/integration tests	test_repair target within the isolated workspace gate	3 passed, 0 failed; public repair regression, MN tag, and help behavior remain green
Core: applicable full workspace gate	TMPDIR=/private/tmp/modkit-u58-test-tmp cargo test --workspace --all-targets --no-fail-fast -- --test-threads=1	183 passed, 14 ignored, 0 failed
Installed-versus-fixed comparison	Inline donor/acceptor fixture from issue #679	Installed 0.6.4 emits one repaired repeat record; fixed code emits zero repaired and one failed record and continues normally
Boundary probe	Query-name-sorted donor and acceptor SAM records with * sequences on exact head	Normal per-record rejection with empty-read-sequence; no Finder access or panic, confirming empty sequences are rejected before substring search
Core: formatting/diff checks	rustfmt --edition 2021 --check modkit-core/src/repair_tags.rs; git diff --check upstream/master...HEAD	Both passed; clean one-file diff, 105 insertions and 8 deletions

Tests not performed

• No large real-data or performance benchmark was run. The change has no performance claim; it replaces collection of non-overlapping matches with at most two linear substring searches.
• No alignment-based disambiguation was tested because selecting among ambiguous placements is explicitly out of scope.

Scientific validation

• Population/eligibility invariant: only records without a unique sequence placement change category, from repaired to existing nonfatal per-record rejection.
• Count/category conservation invariant: the minimal ambiguous fixture changes exactly from one repaired/zero failed to zero repaired/one failed; unrelated records continue.
• Coordinate/strand/interval invariant: a repair is emitted only when the donor-to-acceptor offset is unique in forward-sequence coordinates; reverse-strand controls use the same oracle.
• Determinism invariant: every ambiguous record is rejected rather than selecting an occurrence based on search order.
• Independent oracle or specialist review: occurrence starts 0 and 2 are directly enumerable; Rust and scientific reviews approved the frozen one-file branch.

Output and compatibility

• User-visible change: affected repetitive reads are logged/counted as failed repair records instead of receiving arbitrary projected tags.
• Expected output differences: ambiguous records disappear from the repaired BAM; unique records are unchanged.
• Byte-identical controls: focused unique-placement MM/ML/MN values are exact; existing repair integration tests remain green.
• CLI/API/schema compatibility: no option, public API, header, or tag schema change.
• Partial-output or failure semantics: ambiguity remains a nonfatal record-level rejection.

Reviewer guide

1. Review the parent match_indices behavior on ACACAC/ACAC.
2. Review the two Finder calls in repair_record_pair.
3. Review the reverse and unique-placement controls for unchanged MM/ML/MN behavior.
4. Rerun: cargo test -p mod_kit repair_tags::tests -- --nocapture --test-threads=1.
5. Prefer merging this PR before repair can reorder or omit records and hide fatal BAM failures #680; then rebase repair can reorder or omit records and hide fatal BAM failures #680 so the separate ordering/lifecycle work retains this small scientific correction.

Checklist

• The issue contains reproducible observed and expected behavior.
• The change is limited to the linked issue's approved scope.
• The regression is demonstrably red on the exact parent revision.
• All tests actually performed are listed above with their results.
• Unrun or inapplicable tests are disclosed.
• Scientific counts/statistics and output compatibility are explicitly checked.
• Formatting and diff-hygiene checks pass, or unrelated findings are documented.
• No generated data, private sample identifiers, or unrelated changes are included.

[SuhasSrinivasan]

This completed fix is now consolidated into #711 with the related repair ordering and atomic-output corrections. The combined exact head was independently reviewed and passed the full serial workspace gate (204 passed, 14 ignored, 0 failed). Closing this duplicate review surface so the repair code can be reviewed in one PR.