validate silently accepts invalid or conflicting truth BED rows

[SuhasSrinivasan]

Summary

modkit validate silently drops malformed or unreadable ground-truth BED rows and accepts semantically invalid coordinates, strand tokens, and modification labels. Valid rows that assign conflicting labels to the same (contig, strand, position) silently overwrite one another, so reversing row order changes the full scientific contingency table.

The command can exit successfully and overwrite a requested report even though part of the truth input was rejected or interpreted differently by row order. This reproduces with modkit 0.6.4 and current upstream revision 5cecc3fb3a9336068d9e3c68d5c08d678153dd2c.

Severity

Severity: High — scientific correctness and output integrity

Rationale: Malformed truth annotations can be silently omitted and conflicting valid annotations can change scientific counts by input order while the command reports success. Because output is created before truth validation completes, an existing report can also be truncated before an input error is surfaced.

User and scientific impact

• Affected result or workflow: every modkit validate workflow using BED6 ground truth.
• Direction of error: silent row loss, invalid interval acceptance, last-row-wins relabeling, order-dependent contingency counts, and premature output replacement.
• Likely exposure: data-dependent; malformed mixed files and merged/overlapping truth annotations are realistic inputs.
• Detectability or workaround: prevalidate every token and detect interval overlaps externally before running modkit; compare both row orders to detect conflicting labels. The command's zero exit status is not sufficient.

Affected versions and environment

• Released version: modkit 0.6.4
• Development revision: 5cecc3fb3a9336068d9e3c68d5c08d678153dd2c
• Operating system and architecture: macOS 26.6, arm64
• Input format: BED6+ ground truth and BAM
• Reference test input: tests/resources/input_5mC.bam from the modkit repository

Steps to reproduce

Run these commands from a modkit source checkout. The fixtures are synthetic and contain no private data.

1. A malformed row is silently dropped

Create malformed-truth.bed:

oligo_1512_adapters	9	10	m	.	+
not-a-bed-row

printf 'sentinel\n' > malformed-report.tsv

modkit validate \
  --bam-and-bed tests/resources/input_5mC.bam malformed-truth.bed \
  --canonical-base C --filter-threshold 0 --threads 1 \
  --suppress-progress --out-filepath malformed-report.tsv

printf 'exit=%s\n' "$?"
head -n 1 malformed-report.tsv

Observed with modkit 0.6.4:

• Exit status is 0.
• The log reports Processed 1 BED lines; line 2 is silently discarded.
• The sentinel is overwritten with a successful report based only on line 1.

Expected: a nonzero exit before output creation or modification, with the BED path, physical line 2, and Invalid number of fields root cause. The sentinel remains byte-identical.

2. Conflicting valid labels are row-order dependent

Create conflict-mh.bed and conflict-hm.bed:

# conflict-mh.bed
oligo_1512_adapters	9	12	m	.	+
oligo_1512_adapters	10	11	h	.	+

# conflict-hm.bed
oligo_1512_adapters	10	11	h	.	+
oligo_1512_adapters	9	12	m	.	+

modkit validate \
  --bam-and-bed tests/resources/input_5mC.bam conflict-mh.bed \
  --canonical-base C --filter-threshold 0 --threads 1 \
  --suppress-progress --out-filepath conflict-mh.tsv

modkit validate \
  --bam-and-bed tests/resources/input_5mC.bam conflict-hm.bed \
  --canonical-base C --filter-threshold 0 --threads 1 \
  --suppress-progress --out-filepath conflict-hm.tsv

Observed with modkit 0.6.4: both commands exit 0, but the first full table assigns the overlapping position to ground-truth h, while the reversed file assigns it to m. The first report contains separate h and m rows; the second contains only m.

Expected: both orders fail before output creation or modification. The diagnostic identifies the file, current line, oligo_1512_adapters:10, strand +, the existing label and its first line, and the new label. Identical duplicate/overlapping labels remain idempotent, and the same coordinate on the other strand remains independent.

Additional invalid-token matrix

Each of these rows should fail with its physical line and a specific reason instead of being accepted or silently dropped:

chr1	-1	1	m	.	+       # negative start
chr1	5	4	m	.	+        # reversed interval
chr1	5	5	m	.	+        # zero-width interval
chr1	5	6	m	.	+junk    # strand suffix
chr1	5	6	.	.	+        # invalid modification token
chr1	5	6	mm	.	+       # multi-character short code

Valid blank lines, leading-whitespace comments, documented one-character modification codes, numeric ChEBI identifiers, canonical -, extra BED columns, identical overlaps, and different strands should remain accepted.

Expected behavior

• Every nonblank, noncomment BED row is either parsed completely or returns a contextual error; no row/read error is discarded.
• Coordinates satisfy 0 <= start < end, strands are exactly + or -, and truth labels match the documented grammar.
• Conflicting labels for one (contig, strand, position) fail deterministically in either row order; identical assignments are idempotent.
• All truth inputs validate before the output path is created or modified.

Root-cause evidence

• BED line read/parse errors are converted to None by .ok() inside filter_map at subcommand.rs:182-204.
• The parser does not enforce nonnegative/nonempty intervals and consumes only the first strand character at subcommand.rs:128-158.
• Truth positions use unconditional BTreeMap::insert, which makes a conflicting later row overwrite the earlier label at subcommand.rs:205-212.
• The requested output file is created before truth BED parsing begins at subcommand.rs:900-904.

Proposed fix scope

• Enumerate physical lines explicitly and return open, read, and parse errors with path and line context.
• Validate half-open coordinates, exact strand tokens, and documented modification tokens; preserve numeric-first ChEBI parsing.
• Detect conflicting per-position assignments with compact interval-level provenance while treating identical overlaps as idempotent.
• Parse all truth BEDs and derive the canonical base before creating the output file.
• Align the validate guide with the longstanding exact +/- strand model.

Non-goals

• No new unstranded (.) truth mode or UCSC track/browser directive support.
• No change to validate observation accounting, thresholds, balancing, or BAM processing.
• No redesign of BED storage beyond avoiding temporary position vectors and retaining compact conflict provenance.

Acceptance criteria

• The malformed and conflicting minimal reproducers exit nonzero on the fixed revision and leave existing output byte-identical.
• Diagnostics include file, physical line, and causal parse reason; conflict diagnostics also include coordinate, strand, old/new labels, and original line.
• Negative, reversed, and zero-width intervals; invalid strand suffixes; malformed labels; and read errors are rejected.
• Blank/comments, documented labels, numeric ChEBI codes, extra columns, identical overlaps, and different strands remain valid.
• Valid existing validate output remains unchanged.
• Focused tests and the full workspace test suite pass.

Reproduction artifacts

Artifact	Size	SHA-256	Notes
malformed-truth.bed	45 B	9623a1d4851bca0b68a64fca9c3be19c7f93424e647bc948f01ef1e700fb4671	One valid row followed by one malformed row
conflict-mh.bed	63 B	cadb591c89eb53ca01791daa9d4dc4425efa0acded4630a16ca59c2ffc699bc8	m interval then conflicting h subinterval
conflict-hm.bed	63 B	e2bb5d83a3479f3584fd6f2124fb7545d47b2997f8b45e912d175bc1306a6823	Same assignments in reverse order
Sentinel report	9 B	b5f7e7d285029324d9b3acae19cc05099271454ac98bfc059a92b0581625cd51	sentinel\n; unchanged after every fixed failure

Related work

• Related issue: validate can omit later truth sets and eligible observations #657 covers validate BAM/truth-set lifecycle and observation accounting as a distinct scientific classification review surface.
• Proposed PR: Reject invalid and conflicting validate truth rows #660
• Deferred policy or upstream dependency: none.

[SuhasSrinivasan]

The confirmed validate fix is now included in the consolidated, ready-for-review PR #659. Closing this standalone issue in line with the maintainer's request to keep the identified bug and its code change together in the PR.