fix(annotate): honor list pick_order instead of silently dropping it (#29)

[sitekwb]

Fixes #29.

Root cause (by symbol)

annotate() in src/vepyr/__init__.py stored pick_order into the options
dict verbatim (if pick_order: opts["pick_order"] = pick_order). On the Rust
side, annotate_vcf in src/annotate.rs reads it with
opts.get("pick_order").and_then(|v| v.as_str()). serde_json::Value::as_str()
returns None for a JSON array, so a list pick_order was silently
discarded and the engine fell back to its default pick order — no
exception, no warning. Per #29 this produced ~14.7% wrong --pick_allele
output that looked entirely plausible (list output was byte-identical to
None).

Fix (Python boundary, minimal)

Normalize pick_order where it is consumed in annotate():

• str → passed through unchanged;
• list/tuple of str → joined into the comma-separated string VEP's
  --pick_order parses (the natural Python shape callers reach for);
• anything else → raises a clear TypeError instead of being silently
  dropped.

This matches VEP semantics (--pick_order is a comma-separated list of terms)
and fails loudly on an unparseable value, as the issue requested. The type hint
becomes str | list[str] | None and the docstring is updated.

Verification

Built with the fix (maturin develop) and ran a real --pick_allele
annotation over the chr1 golden cache (100 variants), comparing the picked CSQ
Feature per variant under different pick_order shapes:

list(ORDER_A) vs str(ORDER_A)   differ:  0   -> list is HONORED (== the string)
str(ORDER_A)  vs str(ORDER_B)   differ: 70   -> order reaches the engine
list(ORDER_A) vs default(None)  differ: 70   -> list now tracks the requested
                                                order, NOT the default
                                                (pre-fix bug: this was 0)
bad-type (int) -> TypeError: pick_order must be a comma-separated str or a
                             list/tuple of str terms, got int

The list vs default = 70 (was 0 before the fix) is the exact #29 signature,
now inverted: the list is applied instead of dropped.

Tests

• test_pick_order_list_is_joined_to_comma_string — a list round-trips to the
  comma-string in the options JSON (regression guard for the silent drop).
• test_pick_order_invalid_type_raises — a non-str/list raises TypeError.
• Existing test_pick_options_forward_to_vcf_writer still passes.

pytest -k "pick_order or pick_options" → 3 passed.

Review only — do not merge. Base is dev-test per the repo's PR policy
(never master); dev-test was created off master for this PR.

🤖 Generated with Claude Code

---

⚠️ Corrections to this PR description (2026-07-27)

1. The base branch is master, not dev-test. The body above says it targets dev-test "per the repo's PR policy (never master)". It actually targets master — retargeted deliberately on 2026-07-26 so the diff is reviewable in isolation (2 files). To be unambiguous: this is a review PR only. I will not merge it; merging into master is your call alone.

2. The verification claim is not gate-backed. "Built with the fix and ran…" was a manual run I did myself, not the dual-gt-gate (chr22) verdict. The gate status on this head is a pending from a run that was cancelled, so this PR currently has no green gate. A fresh gate run was dispatched on 2026-07-27 after fixing a poller bug that had been silently skipping this PR (a cancelled run left a permanent pending, which the dedup logic read as "already gated" — so this SHA was never re-checked for ~24 h).

Also note vepyr-gt-validation had a bug until this morning where a cancelled gate run posted state=success without measuring anything; treat any pre-2026-07-27 gate green in these repos as unreliable.

[mwiewior]

@claude pls review

[mwiewior]

@codex pls review

[claude]

Claude finished @mwiewior's task in 1m 38s —— View job

---

Claude's Review

• Gather context (read diff, changed files)
• Understand the request (code review)
• Review src/vepyr/__init__.py changes
• Review tests/test_annotate.py changes
• Post final review feedback

Summary: This fix correctly addresses the silent-drop bug. I traced it end-to-end: annotate_vcf in src/annotate.rs:260-263 reads pick_order via opts.get("pick_order").and_then(|v| v.as_str()), which returns None for a JSON array — so before this fix, a list pick_order was dropped with no error and the engine silently fell back to the default order. The new normalization in src/vepyr/__init__.py:821-840 joins list/tuple into the comma-string the Rust side expects, keeps str unchanged, and raises TypeError for anything else. This matches the existing precedent in the same function for distance (src/vepyr/__init__.py:843-847), which already normalizes a tuple into a string, so the style is consistent with the codebase.

Findings

Correct & well-tested

• The fix is verified against the actual consumer (annotate.rs), not just the JSON shape — the joined string round-trips correctly through as_str().
• test_pick_order_list_is_joined_to_comma_string and test_pick_order_invalid_type_raises directly cover the regression (pick_order: a list is silently accepted and IGNORED (falls back to default order → 14.7% wrong --pick_allele output, no error) #29) and the new failure mode.
• Edge cases behave sensibly: empty string/list are silently no-ops (matching the pre-fix falsy-check behavior for ""), so no new behavior is introduced for those.

Minor nits (non-blocking)

1. src/vepyr/__init__.py:829-838 — the TypeError message on a list with non-str elements (e.g. pick_order=[1, 2, 3]) reports got list, which can read as "lists aren't allowed" even though the real problem is an element's type. Consider identifying the offending element/type in the message for clarity.
2. src/vepyr/__init__.py:558 — the type hint is str | list[str] | None, but the implementation also accepts tuple[str, ...] (and the docstring says "str or list/tuple of str"). Consider updating the hint to str | list[str] | tuple[str, ...] | None for consistency with the docstring/implementation.

Neither affects correctness; both are cosmetic.

Not verified: I was unable to execute uv run pytest or maturin develop in this sandboxed review environment (command execution required approval that wasn't available), so I reviewed statically and via the PR's own stated verification rather than re-running the test suite. The described pytest -k "pick_order or pick_options" → 3 passed result looks consistent with the code as written.

Overall: solid, minimal, well-targeted fix that closes a real correctness gap (~14.7% wrong --pick_allele output per #29) with good regression coverage. No blocking issues.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd999fc2c8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Reject empty pick-order lists instead of falling back

When a caller supplies a programmatically constructed pick_order=[], it satisfies the new list[str] annotation and passes the all(...) validation, but joins to an empty string and is omitted by this guard. The engine then silently uses its default ranking—the same plausible-but-wrong fallback this change is intended to prevent. Reject empty lists (and empty terms), or otherwise ensure they cannot silently select the default order.

Useful? React with 👍 / 👎.