feat(merge): add pangraph merge command

[mmolari]

Adds pangraph merge <LEFT_GRAPH> <RIGHT_GRAPH>, which combines two existing pangenome graphs into one without rebuilding from FASTA. The main use case is extending a graph with new genomes: build a graph for the new sequences, then append it to the existing one.

This branch integrates five PRs: #191, #192, #193, #195, #196.

Changes to already-released commands:

• fixing build single-sequence panic
• more strict duplicate-name and empty-name rejection
• made --verify more strict

[ivan-aksamentov]

⚠️ AI-generated content below. Verify all claims.

#197. feat(merge): add pangraph merge command

• Head: 8f5d2f8e48419bceaf47204fa2d2c2184d29d02b on feat/merge
• Base: master
• Scope: adds the pangraph merge subcommand, extracts shared alignment options into GraphMergeParams, rebuilds sequence verification around name-keyed matching, and hardens build/reconstruct/simplify against unnamed, duplicate, and empty genome names.

Overview

Click to expand

pangraph merge <LEFT> <RIGHT> joins two existing graphs without rebuilding from FASTA, mainly to append new genomes to an existing graph. Two independently built graphs are made id-disjoint by re-hashing one graph's block and node ids (Pangraph::make_disjoint_from), then verification matches genomes by name rather than by record index. The change spans the CLI layer (new merge command, GraphMergeParams extraction), the graph model (relabel, is_id_disjoint_from, sanity_check id/key checks), a new pangraph::reconstruct module, neighbor-joining guards for 0 and 1 input genomes, and two new integration test files.

Observed

The diff matches its description: it adds merge, moves shared alignment options out of build_args into GraphMergeParams, and replaces index-based verification with name-keyed matching. The PR body notes three changes to already-released commands (single-sequence build panic fix, stricter duplicate/empty-name rejection, stricter --verify); all three are present and are exercised by tests. One correctness item stands out for a release build (F1). The id-namespacing machinery this PR introduces (make_disjoint_from, relabel) is superseded by the follow-up #199, so F3 and parts of F4 apply only if #197 is considered on its own.

Blocking issues

Correctness items worth resolving before this ships on the reconstruct path.

🔴 F1. `reconstruct_path_sequence` panics on a malformed graph in release builds [click to expand]

reconstruct_path_sequence looks up the first node by direct map indexing and rotates the genome by that node's stored position [src]. graph.nodes[first_node_id] panics with no entry found for key when a path references a node the map lacks, and genome.rotate_right(first_node_pos) panics when first_node_pos > genome.len() (the preceding length check only proves genome.len() == path.tot_len(), not that the stored offset is in range). The sibling reconstruct_block_sequence in the same file uses the guarded .get().ok_or_else(...) form, so the guarded shape is the intended one.

Effect: pangraph reconstruct reads a graph straight from JSON with no sanity_check, and pangraph merge --verify reconstructs the two input graphs it read from disk. sanity_check runs only under #[cfg(debug_assertions)], so in a release binary a malformed input graph aborts the process with no indication of the offending file, instead of the clean report the sibling function already produces.

Suggestions:

• Resolve the first node with graph.nodes.get(first_node_id).ok_or_else(|| make_internal_report!(...))?, mirroring reconstruct_block_sequence.
• Validate first_node_pos <= genome.len() before rotating and return make_error!(...) otherwise (a malformed external graph is bad input, not an internal bug).
• Add a unit test that builds a graph whose path lists a NodeId absent from nodes, and a second case with a first-node position greater than tot_len, asserting a reported error rather than a panic.

Non-blocking issues

Test deficiencies, robustness, and convention drift. Fix if time allows.

🟡 F2. Error tests match message substrings instead of `assert_error!` [click to expand]

Every new error-path test asserts a hand-picked substring of the rendered message (assert!(report_to_string(&result.unwrap_err()).contains("..."))) rather than the project's exact-match assert_error!, which the new neighbor-joining tests in the same diff already use. Representative sites: reconstruct.rs [src] test module, itest_merge.rs, itest_reconstruct.rs, and simplify_run.rs.

Effect: the loosest cases (.contains("missing"), .contains('a'), .contains('3')) barely constrain the error and keep passing if the function starts failing for an unrelated reason; the error variant and source chain go unchecked. The project's Rust testing convention is to avoid partial error-string matching when an exact-match helper exists.

Suggestions:

• Replace each .contains(...) assertion with assert_error!(result, "<full expected message>") for deterministic-input cases; run once and paste the printed actual message.
• Where the message embeds run-dependent data, assert the full message with the interpolated value substituted, or pin the fixed prefix, so the variant and phrasing are still checked.

🟡 F3. Id-namespacing retry: underived bound, asymmetric collision handling, untested control flow [click to expand]

make_disjoint_from retries relabeling up to a hardcoded MAX_ATTEMPTS = 8 times [src]. Three related concerns:

• The bound 8 is not derived from the id width or a target failure probability; any value from 2 upward behaves identically at 64-bit width.
• The retry recovers only from cross-graph collisions (is_id_disjoint_from false). A within-graph self-collision hits relabel's length-invariant internal_error [src] and propagates out of the loop unretried, even though the same salt variation would resolve it. Severity is coupled to id() truncating XxHash64 to usize: negligible on 64-bit, plausible on 32-bit.
• The retry and exhaustion branches are unreachable by any test (both make_disjoint_from tests succeed on the first attempt).

Effect: correctness rests on a probabilistic argument where a collision-free construction is available, and the only non-trivial control flow in the method is unverified. In the rare within-graph collision case a merge a different salt would complete instead aborts.

Note: this machinery is removed by #199, which derives ids from genome names. If #197 lands independently, address it here; if it lands stacked under #199, this becomes moot.

Suggestions:

• Make namespacing collision-free by construction (a monotonic merge-generation salt never reset by simplify), removing the retry loop.
• Or fold the within-graph check into the same retry, and document why 8 is sufficient (per-attempt collision probability at the id width).
• If kept, add a test that forces a first-attempt collision and asserts recovery on a later attempt.

🟡 F4. New logic and error branches lack tests [click to expand]

Several new or reachable branches are untested:

• find_duplicates [src], the shared basis of every duplicate-name check, has no direct unit or property test; only indirect coverage through callers' message assertions.
• merge_cmd_preliminary_checks empty-input-graph guard [src] and the circularity-mismatch warning [src].
• check_alignment_backend_available [src], both branches.
• The node and path id-vs-key guards in sanity_check [src] (only the block guard is exercised).
• PangraphBlock::relabel "aligns node not in graph" error [src], and the "path not found" / length-mismatch invariants in reconstruct_genome [src].

Effect: user-facing precondition guards and a new pure primitive have no regression coverage; a future refactor could drop a guard or reorder find_duplicates's output with no failing test.

Suggestions:

• Add a parameterized find_duplicates test (empty, no-dup, value repeated 3+ times reported once, sorted order) plus a property test.
• Add direct tests for the empty-input, backend-availability, node/path key-mismatch, and PangraphBlock::relabel branches; parameterize the three sanity_check guards over entity kind.

🟡 F5. Genome-name uniqueness ignores surrounding whitespace [click to expand]

check_genome_names [src] and check_sequence_names [src] reject names that are empty after trim() but detect duplicates on the raw, untrimmed name. So " a" passes the empty check and is treated as distinct from "a".

Effect: genome names differing only by surrounding whitespace slip through. They are then hard to address via reconstruct --verify (matched by exact name) or simplify --strains, and round-trip to FASTA headers with invisible leading or trailing spaces.

Suggestions:

• Detect duplicates on the trimmed name as well, so near-duplicates collide.
• Or document explicitly that surrounding whitespace is significant in genome names, so downstream tooling compares byte for byte deliberately.

🟡 F6. `argmin` aborts guide-tree construction on a NaN distance [click to expand]

pair() selects the closest cluster pair with let iota = Q.argmin()? [src]. ndarray_stats::QuantileExt::argmin returns Err(MinMaxError::UndefinedOrder) when any element lacks a total order, which is the case for a NaN. If any mash distance is NaN (for example a degenerate comparison), Q inherits it and argmin()? aborts the whole build run with an internal-looking error rather than a defined behavior. The diagonal INFINITY sentinel is fully ordered and is not the concern.

Effect: guide-tree construction fails with an opaque UndefinedOrder on NaN input instead of a diagnosable domain error.

Suggestions:

• Guard the distance matrix after calculate_distances(): assert finiteness and return an actionable error naming the offending genome pair.
• Or, if NaN is a legitimate "no data" sentinel, switch to a NaN-aware reduction and document the skip semantics rather than coercing NaN to a finite value.

🟡 F7. Weak assertions in new merge integration tests [click to expand]

itest_reconstruct_verify_merged_graph_ignores_order [src] ends with reconstruct_run(...)?; Ok(()) and no explicit assertion; it relies entirely on verify_args setting verify: Some(..), so it would pass vacuously if that were ever None. Several merge tests assert only a path or block count: itest_merge_single_genome and itest_merge_appends_to_an_already_merged_graph check only merged.paths.len() [src], and test_relabel_keeps_graph_consistent uses a > 1 proxy that a hashed id could in principle violate.

Effect: tests whose names promise a behavior (id survival, reordering independence) verify only that the run did not error.

Suggestions:

• Make the verify test's success explicit and assert the fixture order differs from the graph's path-id order, so it cannot degrade into a no-op.
• Add one content assertion where the test name implies one (reconstructed sequence of the appended genome; id-disjointness of the sub-graphs). Replace the > 1 proxy with != BlockId(0) / != BlockId(1).

🟡 F8. Broken documentation link shipped in `--help` and the generated reference [click to expand]

https://pangraph.readthedocs.io/en/stable/ returns 404 and appears in the root --help doc block, which root_args.rs rewrote in this branch, and is echoed into the generated docs/docs/reference.md [src].

Effect: users following the documentation link from pangraph --help reach an unavailable page.

Suggestions:

• Point the link at the live Docusaurus documentation URL, or remove the line if no stable public docs URL exists yet.

🔵 F9. Convention drift in new code and tests [click to expand]

• The three new sanity_check id/key-mismatch errors use bare eyre::eyre! [src] instead of the project's make_error! / make_internal_*.
• New tests place the actual value first: assert_eq!(merged.paths.len(), 6) [src], inverting the expected-first convention pretty_assertions relies on.
• Three build-focused tests live in itest_merge.rs [src] rather than a build integration file.
• Near-identical two-genome fixture builders exist in colliding_graph [src] and two_genome_graph [src] and drift already (one uses Option names).
• clap field-declaration order is load-bearing: merge_params must be the last flattened field [src] for the "Alignment" help section to claim the right arguments. This is well mitigated by a pinning test [src].

Suggestions: use the project error macros; swap assertion argument order; move build tests or rename them; unify the fixture builders.

🔵 F10. ndarray usage nits in neighbor-joining [click to expand]

test_create_Q_matrix and test_dist compare f64 arrays with exact assert_eq! [src] rather than approx (the ndarray approx feature is already enabled); the current integer-valued inputs are exactly representable, so this is latent fragility, not a live failure. create_Q_matrix and dist build chains of array temporaries and subtract a transposed view from C-order operands, and index the distance matrix with the panicking Index impl. All are bounded by matrix size (genome count) and safe on the current call graph.

Suggestions: use assert_abs_diff_eq! with the tightest passing 1e-N; fuse the Q/dn expressions with Zip/azip!; keep the Q/D shape-equality invariant that makes the indexing safe.

🔵 F11. Redundant work on cold and debug-only paths [click to expand]

reconstruct_path_sequence collects the genome buffer with no path.tot_len() capacity hint [src] though the final length is known, so the backing Vec reallocates as blocks are appended (the buffer is megabase-scale per genome). Under a debug build --verify, every clade genome is reconstructed at each guide-tree node, giving O(L * n log n)..O(L * n^2) reconstruction work; release and non---verify runs are unaffected. verify_graph_against_graphs validates source names once and then re-validates them per source via path_ids_by_name [src] (negligible, two sources).

Suggestions: reserve path.tot_len() before extending (Seq::with_capacity exists); accept or bound the debug-verify reconstruction; build each source's name map inline without re-validating.

🔵 F12. AI-writing tells and prose nits [click to expand]

New Rust doc comments carry U+2014 em dashes (reconstruct.rs, root_args.rs); the new tutorial uses a Unicode arrow; a merge comment uses a transition-word connector; and the CHANGELOG entry has a grammar slip ("options compare" / "sequence"). The generated docs/docs/reference.md carries ~82 em dashes and continuation arrows emitted by the help-markdown generator.

Suggestions: replace em dashes with -- and the arrow with -> in authored files; fix the CHANGELOG grammar; normalize the generator output in print_help_markdown.rs (or exempt the generated reference from the ASCII gate) rather than hand-editing the committed file.

Validation summary

Validation checks [click to expand]

• clippy --all-targets -Dwarnings on the tree containing this code: Pass
• Full nextest suite run to completion in this review: not run (evidence rests on static inspection plus the committed unit and integration tests)
• Deleted dev binary src/bin/merge_two_graphs.rs has no remaining references: Pass
• Concurrency: no new shared mutable state; parallel regions preserve output order: Pass

Notes

Click to expand

• N1. simplify_run drops a #[allow(unused_must_use)] that had silently discarded the Result of simplify, so simplification failures now propagate with ?; the former unwrap on an unnamed path (which panicked) is replaced by a reported error, covered by a new test.
• N2. Name-keyed verification fixes the index-based matching bug: a merged graph whose path ids are renumbered now verifies without a spurious equal-length mismatch, and the streaming loops hold at most one or two genomes in memory.
• N3. GraphMergeParams moved out of commands/build/build_args.rs, so graph_merging, reweave, reconsensus, map_variations, and pangraph_block no longer depend on a CLI args type. mod.rs files stay declaration-only.
• N4. No concurrency defects: the parallel regions are pure map+collect over disjoint work sharing an immutable Send + Sync GraphMergeParams; merge output is independent of thread count.
• N5. sanity_check now verifies each entity's stored id equals its map key before the structural checks, closing the exact gap that relabel depends on.