Finish arena ownership: pair intensities with geometry and filter decoys at seal

[jspaezp]

Current status

Partially implemented: TargetColumnsBuilder now owns mutable construction and consumes itself in seal, producing TargetColumns. Public callers cannot query an unsealed arena or seal one twice.

seal now rejects DecoyPolicy::Force when stored decoys remain (ForceWithStoredDecoys). The previous hole producing derived variants over shipped decoys is closed. Readers still perform the actual filtering; seal does not compact rows.

Remaining work

TargetTable::Mzpaf { geom, frag_intens } still stores intensities outside the geometry. Readers/builders accumulate the sidecar in parallel by convention. The shared builder should own both so row insertion, shard merging, and filtering cannot desynchronize them.

With ownership unified, seal can implement the Force drop itself. Reader-side decoys.accepts(...) can remain an early parse-cost optimization, rather than being required for successful sealing. Preserve labels, m/z, intensities, source IDs, and competition groups together during compaction.

Decide the concrete API around the existing TargetColumnsBuilder; no second builder split is needed. Geometry-only inputs must remain supported without synthesized intensities.

Acceptance criteria

• Structural alignment between fragment geometry and optional intensities at insertion/merge/seal.
• Force drops shipped decoy rows at seal even if a reader did not prefilter them.
• Tests cover compaction with source IDs, competition groups, and intensity sidecars, plus geometry-only inputs.
• All registry readers and the prediction sink use the shared ownership contract.

Separate decisions

The write-only structured-modification columns and charge embedded in seq_mod still warrant review, but are not prerequisites for sidecar ownership. Avoid bundling sequence representation or result-identity redesign into this change.

Related #111 validates the scoring-library boundary and protects it with immutable accessors; it does not move ownership into the shared builder. #97 handles capability/degradation reporting and execution planning.

[jspaezp]

Partial progress landed on this branch; keep this issue open.

Completed:

• TargetColumnsBuilder is now the mutable construction type.
• TargetColumns is sealed and exposes no public mutation/sealing protocol.
• source ids, competition groups, and capabilities are finalized before scoring
  receives the arena.
• sealing rejects the invalid stored-decoys-plus-Force state.

Still required to close this issue:

• move the fragment-intensity sidecar into the builder;
• make builder sealing compact shipped decoys and sidecar values under Force;
• remove reader-level DecoyPolicy::accepts calls once seal owns that invariant;
• decide whether the unused structured-modification columns belong in the new
  builder or should be deleted.

[jspaezp]

Design detail for this, from a review pass on feat/mzspeclib-interchange. Agreeing with the shape proposed above and with rejecting the state parameter, for the same reason: TargetColumns<IonAnnot, Sealed> gates methods, not fields, so pending_ids would still sit drained-but-present on a sealed arena and SourceIds::Absent would still have to exist. The fields have to move.

Two claims above are now stale

• The assert!(matches!(self.source_ids, SourceIds::Absent), "arena is already sealed") guard is gone. seal is consuming as of 08a2392, so double-sealing is already unrepresentable. One of the two runtime guards is closed.
• seal_alone_keeps_shipped_decoys_under_force no longer exists. Only a dangling doc reference to it survives, at timsseek/src/data_sources/reference_library.rs:1242 ("see seal_alone_keeps_shipped_decoys_under_force below"). Worth deleting or repointing independently of this work.

Still live: output_id's .expect("sealed targets have source ids") (target_columns.rs:508), the sidecar hand-check in library_file.rs:208, and Force applied per-reader.

Which fields go where

The 20 fields have three lifecycles, and naming them is what makes the split mechanical:

lifecycle	fields
pushed once, read in both states	precursor_mz charge rt_seconds mobility is_decoy frag_off frag_labels frag_mzs seq_strip_off seq_strip_blob seq_mod_off seq_mod_blob
live only while building	pending_ids pending_groups
exist only after sealing	source_ids decoy_groups caps.decoys
dead	mod_off mods mod_registry

So three types, with the shared twelve extracted rather than duplicated:

pub(crate) struct RowColumns<L: KeyLike> { /* the twelve */ }

pub struct TargetArena<L: KeyLike> {          // the builder
    caps: ReaderCapabilities,                  // no `decoys`
    rows: RowColumns<L>,
    frag_intens: Vec<f32>,
    ids: Vec<Option<OwnedSourceId>>,
    groups: Vec<Option<OwnedSourceId>>,
}

pub struct TargetColumns<L: KeyLike> {         // query only, as today
    caps: TargetCapabilities,                  // `decoys` resolved
    rows: RowColumns<L>,
    source_ids: SourceIds,
    decoy_groups: Option<DecoyGroups>,
}

Three consequences worth designing for explicitly

SourceIds loses Absent, becoming Numeric | Text. That deletes output_id's expect rather than explaining it, and collapses source_id() and output_id() into one infallible accessor. Also drop its #[derive(Default)] -- defaulting a state enum is how Absent arrived. Only two external callers to update (query_handle.rs:99, reference_library.rs:208).

caps splits, which retires the placeholder. capabilities.rs currently documents decoys as "a placeholder: seal overwrites it ... nothing reads it before the seal". With ReaderCapabilities / TargetCapabilities the field does not exist until it is resolved, so there is nothing left to document.

seal becomes a move, and the borrow checker takes over an ordering comment. build_decoy_groups and build_source_ids stop being &mut self methods that drain fields and become functions consuming their input:

pub fn seal(self, decoys: DecoyPolicy) -> Result<TargetTable, SealError> {
    let Self { caps, rows, frag_intens, ids, groups } = self;
    let ships = rows.is_decoy.iter().any(|&d| d);
    let (rows, frag_intens, ids, groups) = drop_shipped_decoys_if(decoys, rows, frag_intens, ids, groups);
    let decoy_groups = DecoyGroups::intern(groups, &ids, &rows)?;  // borrows ids
    let source_ids   = SourceIds::build(ids, rows.n_rows())?;      // moves ids
    ...
}

Today a comment reads "Groups first: an undeclared row falls back to its own id, which is still in pending_ids at this point." In this shape intern borrows and build moves, so the ordering is a compile error to get wrong and the comment is unnecessary. The Force compaction slots in ahead of both, which is the point of the issue.

append_arena should come home

Not mentioned above, and it is the sharpest current symptom. serde/diann_speclib_io.rs:648 does

let (dst, mut src) = (&mut dst.inner, src.inner);

then hand-merges every column and closes with a 20-field exhaustive destructure -- in a DIA-NN reader. It is the only reason inner is pub(crate), and the destructure is an explicit admission that adding a column means editing a format reader.

It becomes TargetArena::append(&mut self, other: Self) in target_columns.rs, inner goes private, and the reader becomes a .reduce(..). Keep the exhaustive destructure -- that property is worth having -- but next to the fields it enumerates, so the decision lands on whoever adds the column.

Measured blast radius, smaller than it looks: .inner is reached from exactly one place, TargetColumns { .. } is named literally in exactly one place (that destructure), and source_id() has two external callers.

Also worth renaming SourceIdError to SealError while there. It now carries ForceWithStoredDecoys and GroupHasTwoTargets, neither about ids.

Row.mods: the decision is made

The issue lists this as a decision to force. It is settled: Row.mods is only ever &[], ModDefinition is constructed nowhere, mod_range has no callers -- write-only on both ends, as suspected. I implemented the deletion (mods, mod_registry, mod_off, ModDefinition, mod_range, the push_row CSR bookkeeping and the append_arena handling), verified it workspace-green at +34/−126 across 13 files, then reverted it unmerged for timing reasons. It costs 4 bytes per row via the mod_off CSR entry. Two traps for whoever redoes it: seq_mod_blob/seq_mod_off/seq_mod_range are live and easy to confuse with these, and removing Row.mods makes three ..Default::default() sites complete, which clippy flags as needless_update.

Doing that deletion first means three fewer fields to place.

Suggested order

Each step compiles and tests green on its own, which matters at this size:

1. Delete the dead mod columns (above).
2. Decide sequence_features. It is a third copy in TargetCapabilities, set optimistically by the reader, corrected by nobody, read by nothing -- the live gate is ReferenceLibrary::gate_sequence_features, which is already library-wide and correctly so. If that copy goes, ReaderCapabilities may be just isotopes and not worth a type.
3. Extract RowColumns. Mechanical, large diff, no semantic change; keep the accessors delegating so nothing outside the module moves.
4. Move ids/groups/frag_intens onto the arena and make seal consume. The invariants land here.
5. SourceIds drops Absent; output_id drops the expect.
6. seal owns the Force compaction; accepts in readers demotes to an optimisation.
7. append onto the arena, inner private, rename the error.

Two things not to "fix" on the way past

decoy_groups: Option should stay. None means every row is its own group, which decoy_group_code derives without storing anything -- a representation choice, not an invalid state. Likewise an empty RowColumns: an empty library is legitimate, and timsseek rejects rows-without-fragments separately.