Skip to content

P3-15′-c-ii: the criterion (c) synteny predicate, its per-candidate producer, and D4's two symmetric diagnostics - #114

Merged
bioedca merged 22 commits into
mainfrom
p3-15c-ii-synteny
Aug 7, 2026
Merged

bioedca merged 22 commits into
mainfrom
p3-15c-ii-synteny

Conversation

@bioedca

@bioedca bioedca commented Aug 7, 2026

Copy link
Copy Markdown
Owner

ADR-0006 D4's criterion (c) over the 339 md5-verified NCBI GFFs P3-15′-c-i acquired: the frozen predicate, the per-candidate producer that writes candidate_id → passed|failed|unavailable, and — not optional, D4 asks for them symmetrically — the false-pass and false-FAIL/per-clade-exclusion/pseudogene diagnostics.

The criterion discriminates, and by a wide margin

arm n (c) pass rate
round-0 candidates 941 58.0 % (546 passed / 96 failed / 299 unavailable)
clade-matched random leaders 9,600 1.82 %
§9.1 5′UTR decoys (re-instantiated in-corpus) 9,097 4.74 %
§9.1 tRNA-adjacent decoys 1,860 2.69 %
positive-context control 4,778 99.35 %

Margin 0.975 ≥ 0.20 ⇒ the control is powered, so the rates are interpretable rather than merely quiet. The 58 % is not looseness: the round-0 pool is Stage-1 false positives on a masked negative substrate, and a 96-nt element scoring 0.9977 sitting 19 bp upstream of an alanine--tRNA ligase is most plausibly an uncatalogued T-box. ADR-0005 D14's spare rule exists so those are spared rather than trained against.

⚠ The biggest defect was found by disbelieving a number, not by a test

The very first run returned 541 of 941 passing while the per-CDS background density of D4's four classes is 4.14 % — irreconcilable. The downstream walk bounded distance only from above, so a window buried inside a long gene (a bacterial genome is ~88 % coding) found that gene and inherited its identity. D4's class-II case genuinely needs a negative distance, so the fix could not be "reject overlap": the CDS start must lie at or after the element's own 5′ end, which pins no new number. Corpus counts barely moved — the number that exposed the bug was not the number the bug corrupted.

⚠ Two silent zeros the diagnostic found in itself

  1. The pseudogene diagnostic reported 0 corpus-wide, because the tandem carve-out hops exactly the population it exists to size. Counts now accumulate across the whole walk (15 pseudogenized, 88 unjudgeable).
  2. D4's hypothetical/pseudogene → Pfam/KO route was entirely unreachable: gene_identity_text collects Dbxref/Name as well as product, and on NCBI CDS those are accessions (Dbxref on 29,852 of 30,273 measured CDS), so every hypothetical protein still had non-empty "identity text" and was scored a decided failed. Identifier-shaped values are now filtered, with a positive control asserting the filter removes identifiers rather than removing judgement.

⚠ The vocabulary is precision-first because the permissive version was measured first

An "amino-acid word + enzyme word" rule swallows HAMP domain-containing histidine kinase (38 CDS), serine/threonine protein kinase, glutamine amidotransferase (purine/His/Trp — not tRNA-dependent transamidation), methylated-DNA--[protein]-cysteine S-methyltransferase, peptide-methionine (S)-S-oxide reductase. Every EXCLUSIONS entry was observed on this corpus first; exclusions apply before every include rule; transamidation rules all require the literal tRNA. Two ordering corrections from the audit: the curated product name outranks the gene symbol (hisJ is the His transporter's binding protein in enterics and histidinol phosphatase in Firmicutes), and the symbol route fires only when the product says nothing — otherwise it put PTS galactitol transporter subunit IIB into transamidation via gatB, cytochrome C-552 into transport via cycA, and homocitrate synthase into aaRS via lysS (3 of its 24 matches wrong).

The four functional classes carry a §10.1 gate with four agreeing sources: DOI:10.1002/wrna.1273, PMID:18359782, PMID:19258532, DOI:10.1073/pnas.1220991110 (accessed 2026-08-07).

⚠ The contig bridge is by identity — an index join would be wrong on half the corpus

:c<ci> indexes FASTA record order; a GFF seqid is the NCBI contig accession. 23 of 48 candidate-carrying annotated hosts order their GFF seqids differently from their FASTA records. The id join resolves 134 of 134 needed (host, contig) pairs.

Three gaps reported, not papered over

  1. The shipped §9.1 decoy rows carry coordinates on 0 annotated production hostsdecoys_v0's three non-structured pools have no accession, its 2,999 structured_rna accessions are Rfam contigs, mining_pool_v0's 45,988 windows sit on TBDB source genomes. That arm is available: false with its counts; the §9.1 leader class is re-instantiated inside the annotated corpus instead.
  2. The false-FAIL arm has 2 TBDB records on 1 assembly — the production substrate is a negative substrate by construction. Reported with its n, never as a rate.
  3. The joint a ∧ b ∧ c arm is withheld — (b) has no backend until P3-15′-d.

Plus: the Pfam/KO fallback DB is an unmet §10.2 acquisition, so HMM_FALLBACK_AVAILABLE = False, hypothetical/pseudogenized ORFs resolve unavailable, and ADR-0005 D14 spares them (fail-closed). The population is counted rather than assumed.

Per-clade exclusion — the cap D4 commissioned, and it is large

31.8 % of candidates unavailable (286 host-unannotated, 11 unjudgeable, 2 contig-absent). Five phyla at 100 % (Chloroflexota, Desulfobacterota, Fibrobacterota, Thermotogota, Bdellovibrionota_C); Bacillota 38.5 %; Atribacterota / Sysuimicrobiota / Fusobacteriota 0 %. That is the recall cap the commissioned bakta/prokka arm exists to lift. ⚠ CORRECTED (review round 1). This description originally said "passing distances p50 117 / p95 269 / p99 364 / max 454 bp" — that was a superseded run, and the committed artifact said max 1652 over n 554. Two coupled defects: the statistic did not filter on the strand detail's own status, and the tandem carve-out re-anchored the search but not the decision. Both fixed; five candidates move failedpassed. Decision distance (what criterion_c judges): p50 117 / p95 266 / p99 356 / max 454 bp, entirely inside the pad. Element-relative distance: p50 117 / p95 272 / p99 389 / max 1652 bp, with n_passed_via_tandem_carve_out: 5 explaining the tail.

Validation

  • full unit+golden 4,621 passed / 13 failed vs a matched baseline worktree at 95d0149 (4,492 / 13) — failure sets identical, comm empty both ways
  • 28 sabotages / 28 RED against their NAMED test
  • both committed reports re-derive byte-identically from the final code apart from provenance
  • ruff 0.15.15 + black 25.11.0 clean; no .smk touched; no envs/* edit; LOCAL only (48 hosts / 120,089 CDS in 5.7 s)

n_mined is still structurally 0

Sparing is a disjunction so mining is a conjunction: relaxed_architecture remains the one unavailable backend, so max_possible_yield caps the round at 0 until P3-15′-d lands. This step removes one of the two blockers, not both.

https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM

Summary by CodeRabbit

  • New Features

    • Added synteny-based candidate evaluation using downstream gene context, strand awareness, distance limits, and configurable intervening-gene rules.
    • Added synteny status tracking across mining and re-mining workflows, including unavailable-state handling and fail-closed validation.
    • Added shard processing, status-table merging, supply inspection, and command-line reporting.
    • Added false-pass and exclusion diagnostic reports with clade, pseudogene, availability, and sensitivity metrics.
  • Tests

    • Added comprehensive coverage for synteny evaluation, diagnostics, validation, configuration, and workflow integration.

⚠ Review round 1 — 20 findings (app 5, CLI 15), all real, 8 major, one in common

The major one: the tandem carve-out reached the search but not the decision. resolve_downstream_gene selected its target with the re-anchored window and then returned that target's element-relative distance, which synteny_status judged against the 500 bp pad — so D4's carve-out fired only where it was not needed and turned exactly the tandem loci it exists for into false FAILs. DownstreamGene now carries both distances and criterion_c sees decision_distance_bp. My own test could not see it: it asserted the walk reached the right gene, which was always true. The search was never wrong; the decision was.

Two artifact-pinning tests went green under sabotage — the campaign's best output. The distance-statistic and provenance tests read the committed report, generated by the fixed code, so breaking the source could not reach them. TestDiagnosticsComputedNotJustCommitted now calls the report builders directly, so the rule is under test rather than one frozen output of it.

A test that could not catch the trap it named. test_no_clause_reads_dvc_tracked_data derived against REPO_ROOT, so on any checkout that has run dvc pull a DVC-path clause would read True and it would still pass. It now derives against a tmp_path root holding only the SUPPLY_CLAUSES files.

The produced table was never read by either round — both reviewers found it independently. --synteny-status is wired into both apply-spare-rule legs, and declaring the backend without a table (or supplying one without declaring it) now raises, the same answer apply_remine_spare_rule's probe_set already got for the same reason.

Also fixed. An absolute /home/<user>/… path in committed provenance (out-of-repo inputs are hashed as {name, sha256}); the unavailability reason scanning both strands under a single-strand policy; merge_status_tables passing two inputs through unvalidated and accepting an empty shard; _upstream_window bounding the plus arm but not the minus; combine_strand_statuses raising a bare KeyError; load_contig_ids raising IndexError on a bare > header; synteny._gene_symbols private while the producer's control arm depended on it.

…per-candidate producer, and D4's two symmetric diagnostics

ADR-0006 D4's criterion (c) over the 339 md5-verified NCBI GFFs P3-15'-c-i acquired.

Inputs
  data/processed/mining/round0_fp_manifest.json (941 candidates / 76 hosts)
  data/interim/production_annotations/*.gff.gz (339, DVC)
  data/interim/production_genomes/*.fna (the contig-id bridge)
  data/processed/mining/production_genomes_v0.parquet (GTDB clade)
  data/processed/negatives/{decoys,mining_pool}_v0.parquet (the 9.1 arm)

Outputs
  src/tbox_finder/mining/synteny.py           criterion_c (ADR-frozen signature), the
                                              downstream walk, the tandem carve-out, and
                                              the gene-identity vocabulary D4 delegates here
  src/tbox_finder/mining/synteny_producer.py  the per-candidate status table + both diagnostics
  reports/p3/synteny_false_pass.json          clade-matched leaders + the 9.1 leader class
  reports/p3/synteny_exclusion_diagnostic.json  per-clade exclusion + pseudogene + distances
  mine_round.SYNTENY_SUPPLY_AVAILABLE, synteny_producer.derive_synteny_supply_available()
  candidate_evidence / remine_candidate_evidence thread a synteny status map
  remine --synteny-available promoted to the two-way pair + exit-4 unevidenced preflight
  tests/unit/test_synteny.py, tests/unit/test_synteny_producer.py (+112 tests)

Measured
  941/941 statuses: 541 passed / 101 failed / 299 unavailable
  false-pass: clade-matched random leaders 1.67%, 9.1 5'UTR decoys 4.52%,
              9.1 tRNA-adjacent 2.48%; positive-context control 99.20% (margin 0.975, POWERED)
  per-clade exclusion 31.8% overall; five phyla at 100%; Bacillota 38.5%
  passing distances p50 117 / p95 269 / p99 364 / max 454 bp, all inside D4's 500 bp pad
  contig bridge is by identity: 23 of 48 hosts order GFF seqids differently from FASTA
  records, so an index join would be wrong on ~48% of them; the id join resolves 134/134

Three gaps reported, not papered over
  the shipped 9.1 decoy rows carry coordinates on 0 annotated hosts (arm unavailable + counts)
  the false-FAIL arm has 2 TBDB records on 1 assembly (negative substrate, by construction)
  the joint a^b^c arm is withheld: (b) has no backend until P3-15'-d
  the Pfam/KO fallback DB is an unmet 10.2 acquisition, so hypothetical/pseudogenized ORFs
  resolve unavailable and ADR-0005 D14 spares them (fail-closed); the population is counted

Validation
  full unit+golden 4,604 passed / 13 failed vs a matched baseline worktree at 95d0149
  (4,492 / 13) - failure sets identical, comm empty both ways
  19 sabotages / 19 RED against their NAMED test
  both committed reports re-derive byte-identically from the final code apart from provenance
  ruff 0.15.15 + black 25.11.0 clean; no .smk touched

n_mined is still structurally 0: relaxed_architecture remains the one unavailable backend,
so max_possible_yield caps the round at 0 until P3-15'-d lands.

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Changes

The PR adds ADR-0006 D4 synteny evaluation, annotation-backed producer workflows, diagnostic reports, and synteny status propagation into mining and re-mining evidence.

Synteny evaluation

Layer / File(s) Summary
D4 synteny backend and validation
src/tbox_finder/mining/synteny.py, tests/unit/test_synteny.py
Adds identity classification, coordinate mapping, downstream CDS traversal, tandem carve-outs, status folding, and unit coverage.

Producer execution and outputs

Layer / File(s) Summary
Producer execution, diagnostics, and reports
src/tbox_finder/mining/synteny_producer.py, reports/p3/synteny_*.json, tests/unit/test_synteny_producer.py
Adds shard status tables, control diagnostics, exclusion diagnostics, supply derivation, CLI commands, provenance output, and validation tests.

Evidence integration

Layer / File(s) Summary
Round and re-mining evidence wiring
src/tbox_finder/mining/mine_round.py, src/tbox_finder/mining/remine.py, tests/unit/test_synteny_producer.py
Propagates per-candidate synteny statuses, defaults missing entries to unavailable, and validates supply declarations and status-table inputs.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant SyntenyProducer
  participant SyntenyEvaluator
  participant GFF3Annotations
  participant StatusTable
  CLI->>SyntenyProducer: Run shard evaluation
  SyntenyProducer->>SyntenyEvaluator: Evaluate candidate locus
  SyntenyEvaluator->>GFF3Annotations: Load contig and CDS annotations
  GFF3Annotations-->>SyntenyEvaluator: Return downstream CDS features
  SyntenyEvaluator-->>SyntenyProducer: Return synteny status
  SyntenyProducer->>StatusTable: Build and validate status table
  StatusTable-->>CLI: Write results and diagnostics
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 52.66% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the criterion (c) synteny predicate, per-candidate producer, and two diagnostics that form the main changes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch p3-15c-ii-synteny

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/tbox_finder/mining/mine_round.py (1)

524-526: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

The docstring no longer matches the signature.

The text says the other two disjuncts stay unavailable because no synteny backend exists at P2. downstream_aaRS_synteny is now produced from synteny_status. Update the bullet to describe the new third mode, and keep the absent-id fail-closed rule stated.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/tbox_finder/mining/mine_round.py` around lines 524 - 526, Update the
docstring around the SpareRuleEvidence handling to reflect that
downstream_aaRS_synteny is now derived from synteny_status, documenting the new
third mode instead of claiming no synteny backend exists at P2. Preserve the
statement that an absent identifier fails closed by being spared rather than
mined.
🧹 Nitpick comments (4)
src/tbox_finder/mining/synteny.py (2)

78-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

__all__ omits two public names that other modules already use.

combine_strand_statuses and excluded_by are used by src/tbox_finder/mining/synteny_producer.py and by the tests, but they are not exported. Add them so the public surface matches the actual contract.

♻️ Proposed export addition
     "classify_gene_identity",
+    "combine_strand_statuses",
     "contig_seqid",
     "criterion_c",
     "downstream_cds_on_strand",
+    "excluded_by",
     "load_contig_ids",
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/tbox_finder/mining/synteny.py` around lines 78 - 98, Update the __all__
declaration in synteny.py to include the existing public symbols
combine_strand_statuses and excluded_by, preserving the current exports and
ordering style.

520-528: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

_gene_symbols is private but is called from another module.

src/tbox_finder/mining/synteny_producer.py line 710 calls synteny._gene_symbols(c). Either promote this helper to a public name, or expose a public wrapper that classifies a CdsFeature directly. That would also remove the duplicated "identity text plus symbols" call pattern in the producer.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/tbox_finder/mining/synteny.py` around lines 520 - 528, Expose the
symbol-extraction behavior from _gene_symbols through a public helper, or rename
it to a public symbol, and update synteny_producer’s call site to use that API
instead of accessing the private helper. Preserve the existing lower-casing,
attribute keys, length limit, and frozenset result while consolidating the
producer’s duplicated identity-text and symbol classification flow.
tests/unit/test_synteny_producer.py (1)

338-348: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a pin for the passing-distance block.

No test checks that passing_distance_sensitivity describes passing rows only. That gap let max_bp: 1652 ship against window_bp: 500. Add an assertion that max_bp is at most window_bp and that n does not exceed the passed count. This test would go red against the current committed report, which is the point.

💚 Proposed test
+    def test_the_passing_distances_are_inside_the_window(self, exclusion: dict) -> None:
+        block = exclusion["passing_distance_sensitivity"]
+        assert block["max_bp"] is None or block["max_bp"] <= block["window_bp"]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/test_synteny_producer.py` around lines 338 - 348, Add assertions
to the passing_distance_sensitivity test coverage ensuring max_bp is no greater
than window_bp and n does not exceed the number of passed rows. Anchor the
checks to the existing exclusion fixture/report symbols and preserve the current
reconciliation assertions.
src/tbox_finder/mining/synteny_producer.py (1)

479-500: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Contig extent is approximated by the last CDS end.

extents uses the maximum CDS end on the contig as the contig length. Random control windows therefore never fall in the region after the last annotated CDS. State this bias in the docstring, or read the real contig lengths from the genome FASTA that HostAnnotation already opens.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/tbox_finder/mining/synteny_producer.py` around lines 479 - 500, Update
the function containing the extents calculation to avoid treating the maximum
CDS end as the contig length: use the real contig lengths from the FASTA already
opened by HostAnnotation when available, and preserve the existing fallback only
if necessary. If the approximation remains, explicitly document this sampling
bias in the function docstring.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@reports/p3/synteny_exclusion_diagnostic.json`:
- Around line 251-254: Update the diagnostic provenance generation for the
inputs recorded in _provenance so committed keys use repository-relative paths
instead of absolute personal temporary paths. Rerun the diagnostics with
synteny_status.json located under a repository-relative location, or normalize
the existing inputs keys before writing the report while preserving their
checksums.

In `@src/tbox_finder/mining/mine_round.py`:
- Around line 508-511: Update candidate_evidence and both spare-rule call paths
to propagate the loaded synteny status map whenever synteny_available=True: pass
it into read_fp_manifest and read_remine_manifest, while preserving None when
synteny is unavailable so existing fail-closed behavior remains correct.

In `@src/tbox_finder/mining/synteny_producer.py`:
- Around line 816-824: Update the distance collection in the loop over
per-strand details, and the corresponding logic around the referenced second
occurrence, to include distances only when the detail’s own status indicates it
passed and its distance is within the configured window. Keep class-count and
other counters unchanged, then regenerate both affected reports so their
passing-distance statistics reflect only passing candidates.
- Around line 291-295: Update the unavailability-reason logic in the
status-folding function so pseudo detection examines only the strand selected by
config.strand_policy when it is plus or minus; retain scanning across both
strands for the policy that considers both, ensuring reason matches the strand
that determined status.

In `@src/tbox_finder/mining/synteny.py`:
- Around line 795-837: Update the downstream-gene selection flow around the
re-anchoring of anchor in the relevant mining function to preserve both the
original element-relative distance and the re-anchored distance. Use the
re-anchored distance when evaluating or passing the target to criterion_c, while
retaining the original distance for the reported DownstreamGene result. Add a
synteny_status regression test covering a tandem carve-out target beyond 500 bp
from the original element but within the re-anchored window.

---

Outside diff comments:
In `@src/tbox_finder/mining/mine_round.py`:
- Around line 524-526: Update the docstring around the SpareRuleEvidence
handling to reflect that downstream_aaRS_synteny is now derived from
synteny_status, documenting the new third mode instead of claiming no synteny
backend exists at P2. Preserve the statement that an absent identifier fails
closed by being spared rather than mined.

---

Nitpick comments:
In `@src/tbox_finder/mining/synteny_producer.py`:
- Around line 479-500: Update the function containing the extents calculation to
avoid treating the maximum CDS end as the contig length: use the real contig
lengths from the FASTA already opened by HostAnnotation when available, and
preserve the existing fallback only if necessary. If the approximation remains,
explicitly document this sampling bias in the function docstring.

In `@src/tbox_finder/mining/synteny.py`:
- Around line 78-98: Update the __all__ declaration in synteny.py to include the
existing public symbols combine_strand_statuses and excluded_by, preserving the
current exports and ordering style.
- Around line 520-528: Expose the symbol-extraction behavior from _gene_symbols
through a public helper, or rename it to a public symbol, and update
synteny_producer’s call site to use that API instead of accessing the private
helper. Preserve the existing lower-casing, attribute keys, length limit, and
frozenset result while consolidating the producer’s duplicated identity-text and
symbol classification flow.

In `@tests/unit/test_synteny_producer.py`:
- Around line 338-348: Add assertions to the passing_distance_sensitivity test
coverage ensuring max_bp is no greater than window_bp and n does not exceed the
number of passed rows. Anchor the checks to the existing exclusion
fixture/report symbols and preserve the current reconciliation assertions.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5c1efa48-0a48-45b3-8ba2-7df6dcadf81d

📥 Commits

Reviewing files that changed from the base of the PR and between 95d0149 and ba04aea.

⛔ Files ignored due to path filters (1)
  • analyses/phase3_log.qmd is excluded by !**/*.qmd
📒 Files selected for processing (8)
  • reports/p3/synteny_exclusion_diagnostic.json
  • reports/p3/synteny_false_pass.json
  • src/tbox_finder/mining/mine_round.py
  • src/tbox_finder/mining/remine.py
  • src/tbox_finder/mining/synteny.py
  • src/tbox_finder/mining/synteny_producer.py
  • tests/unit/test_synteny.py
  • tests/unit/test_synteny_producer.py

Comment thread reports/p3/synteny_exclusion_diagnostic.json Outdated
Comment thread src/tbox_finder/mining/mine_round.py
Comment thread src/tbox_finder/mining/synteny_producer.py
Comment thread src/tbox_finder/mining/synteny_producer.py Outdated
Comment thread src/tbox_finder/mining/synteny.py
…aches the decision, not just the search

20 findings (CodeRabbit app 5, CLI 15), all real, 8 major, one in common.

MAJOR - the carve-out was selectively broken.
resolve_downstream_gene selected its target with the RE-ANCHORED window but returned that
target's ELEMENT-RELATIVE distance, which synteny_status judged against the 500 bp pad. So
D4's tandem carve-out fired only where it was not needed and turned the tandem loci it exists
for into false FAILs. DownstreamGene now carries both distances; criterion_c sees
decision_distance_bp. Five candidates move failed -> passed (541 -> 546 / 96 failed / 299
unavailable). The prior test asserted the walk reached the right gene - always true; the
search was never wrong, the decision was.

MAJOR - the distance statistic counted evaluations that did not pass.
It collected every strand detail whose CLASS was passing without checking its STATUS, so
out-of-window hits that FAILED were counted: 554 entries with a 1,652 bp max in a block
documented as "measured on the passing candidates" against 541 passed. Now filtered on the
detail's own status, and reported as two series - element-relative and the re-anchored
decision distance. Decision distances: p50 117 / p95 266 / p99 356 / max 454, all inside the
pad; the element-relative max is explained by n_passed_via_tandem_carve_out: 5.

CORRECTION - a number reported from a superseded run.
The PR body, the previous commit message and the dev-log quoted p95 269 / p99 364 / max 454
(n 549). The committed artifact said p95 272 / p99 389 / max 1652 (n 554): the field was read
before the identifier-filter fix and not re-read after regenerating.

MAJOR - the produced table was never read by either round.
Both reviewers found it independently. --synteny-status is now wired into both
apply-spare-rule legs, and declaring the backend without a table (or supplying one without
declaring it) raises - the same answer apply_remine_spare_rule's probe_set already got.

MAJOR - a committed provenance record named an absolute /home/<user>/ path.
Out-of-repo inputs are hashed and recorded as {name, sha256} instead.

MAJOR - a test that could not catch the trap it named.
test_no_clause_reads_dvc_tracked_data derived against REPO_ROOT, so on any checkout with
dvc pull run a DVC clause would read True and it would still pass. It now derives against a
tmp_path root holding only the SUPPLY_CLAUSES files.

TWO ARTIFACT-PINNING TESTS WENT GREEN UNDER SABOTAGE.
The distance-statistic and provenance tests read the COMMITTED report, generated by the fixed
code, so breaking the source could not reach them. TestDiagnosticsComputedNotJustCommitted now
calls the report builders directly. Both new tests bite.

Also: the unavailability reason scanned both strands under a single-strand policy;
merge_status_tables passed two inputs through unvalidated and accepted an empty shard;
_upstream_window bounded the plus arm but not the minus; combine_strand_statuses raised a bare
KeyError; load_contig_ids raised IndexError on a bare ">" header; synteny._gene_symbols was
private while the producer's control arm depended on it; the candidate_evidence docstring still
said no synteny backend exists.

Validation
  full unit+golden 4,621 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  28 sabotages / 28 RED against their NAMED test
  both reports re-derive byte-identically from the final code apart from provenance
  ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/tbox_finder/mining/mine_round.py (1)

1304-1314: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Add the synteny supply evidence gate to _cmd_apply_spare_rule.

When args.synteny_available is true, call derive_synteny_supply_available(). Return exit code 4 when the derivation is unavailable, before calling apply_spare_rule. The current path can otherwise pass an unevidenced criterion-(c) backend to run_mine_round.

src/tbox_finder/mining/remine.py (1)

771-797: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Wire the synteny status table into slurm/p3/stage1_remine.sbatch. The remine CLI defaults synteny to available, but this driver omits --synteny-status; the apply step therefore raises before writing the report. Catch this configuration error and return the existing distinct error code. Apply the same guard to the P2 driver if it passes --synteny-available.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/tbox_finder/mining/remine.py` around lines 771 - 797, Update the
apply-spare-rule invocation in slurm/p3/stage1_remine.sbatch to provide the
required --synteny-status input whenever --synteny-available is enabled, and
catch the resulting configuration error so the driver returns the existing
distinct error code instead of aborting before writing the report. Apply the
same argument and error guard to the P2 driver wherever it passes
--synteny-available, while preserving behavior when synteny is unavailable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/unit/test_synteny_producer.py`:
- Around line 453-456: Replace the `--help`-based assertion in the
`apply-spare-rule` test with a meaningful option-recognition check: expose
parser construction through `build_parser()` and inspect the subparser’s option
strings, or parse a complete command and verify an unknown option exits 2 while
`--synteny-status` reaches `_cmd_apply_spare_rule`. Ensure the test fails if
`--synteny-status` is removed.

---

Outside diff comments:
In `@src/tbox_finder/mining/remine.py`:
- Around line 771-797: Update the apply-spare-rule invocation in
slurm/p3/stage1_remine.sbatch to provide the required --synteny-status input
whenever --synteny-available is enabled, and catch the resulting configuration
error so the driver returns the existing distinct error code instead of aborting
before writing the report. Apply the same argument and error guard to the P2
driver wherever it passes --synteny-available, while preserving behavior when
synteny is unavailable.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 78d59d76-5d9d-411f-90d2-cd4fbcb6dda1

📥 Commits

Reviewing files that changed from the base of the PR and between ba04aea and 4b49352.

⛔ Files ignored due to path filters (1)
  • analyses/phase3_log.qmd is excluded by !**/*.qmd
📒 Files selected for processing (8)
  • reports/p3/synteny_exclusion_diagnostic.json
  • reports/p3/synteny_false_pass.json
  • src/tbox_finder/mining/mine_round.py
  • src/tbox_finder/mining/remine.py
  • src/tbox_finder/mining/synteny.py
  • src/tbox_finder/mining/synteny_producer.py
  • tests/unit/test_synteny.py
  • tests/unit/test_synteny_producer.py
🚧 Files skipped from review as they are similar to previous changes (5)
  • reports/p3/synteny_false_pass.json
  • reports/p3/synteny_exclusion_diagnostic.json
  • src/tbox_finder/mining/synteny_producer.py
  • src/tbox_finder/mining/synteny.py
  • tests/unit/test_synteny.py

Comment thread tests/unit/test_synteny_producer.py Outdated
…ntained its own positive class

7 findings (CodeRabbit app 1, CLI 6), all real, 5 major. Counts across two rounds: 20, 7.

MAJOR - the 5'UTR decoy arm sampled the CDS the positive-context arm is built from.
A 5'UTR upstream of an aaRS in a real genome may itself BE a T-box leader, so counting it as
a FALSE pass inflates the rate; excluding it stops the arm representing a random leader.
Reporting one reading only would be a choice disguised as a measurement, so both ship:
over all CDS the arm reads 5.28%, over CDS naming none of D4's four classes it reads 0.35%.

MAJOR - the sub-threshold hop could skip a CDS that already satisfied the criterion.
The docstring already claimed the carve-out fires only on an ORF that cannot carry the
criterion either way; the length branch did not enforce it. NO candidate verdict changed on
this corpus - the case does not occur here - so this is latent correctness the new test pins,
recorded as such rather than dressed up as a fix that moved a number.

MAJOR - my CLI-reachability test was vacuous.
It parsed [..., "--synteny-status", "t.json", "--help"] and asserted exit 0, but argparse
handles --help BEFORE rejecting an unknown option, so that exits 0 for a flag the parser has
never heard of (verified by execution). mine_round.build_parser was extracted so the
assertion is structural, plus a negative control that the option set can answer False.

MAJOR - exclusion_reason_totals broke the closed reason vocabulary.
It fell back to "" while the per-clade breakdown fell back to host_unannotated for the SAME
row. Both now read one derivation, which raises on an unknown reason.

MAJOR - the merge did not preserve the shard-recorded hmm_fallback_available.
config.as_dict() re-reads the LIVE constant, so a merge on a checkout where the flag had
flipped would rewrite what the shards ran under. The shard-recorded value wins.

MINOR - shipped_decoy_availability returned a partial shape when pandas is absent; a test
asserted decision max_bp non-None but compared block max_bp without the same guard.

Validation
  full unit+golden 4,627 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  32 sabotages / 32 RED against their NAMED test
  ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@reports/p3/synteny_false_pass.json`:
- Around line 13-30: The class-excluded diagnostic must be derived by filtering
the wide-arm candidate population so its row count cannot exceed the wide arm;
regenerate reports/p3/synteny_false_pass.json (lines 13-30) or revise its
wording so the rates are not presented as one arm and its subset. Add a
validation in tests/unit/test_synteny_producer.py (lines 317-325) asserting
strict["n"] <= wide["n"] to catch future inconsistencies.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 19781ecd-35fc-4fd9-ad91-4a3283032778

📥 Commits

Reviewing files that changed from the base of the PR and between 4b49352 and b61fd3a.

⛔ Files ignored due to path filters (1)
  • analyses/phase3_log.qmd is excluded by !**/*.qmd
📒 Files selected for processing (7)
  • reports/p3/synteny_exclusion_diagnostic.json
  • reports/p3/synteny_false_pass.json
  • src/tbox_finder/mining/mine_round.py
  • src/tbox_finder/mining/synteny.py
  • src/tbox_finder/mining/synteny_producer.py
  • tests/unit/test_synteny.py
  • tests/unit/test_synteny_producer.py
🚧 Files skipped from review as they are similar to previous changes (4)
  • reports/p3/synteny_exclusion_diagnostic.json
  • tests/unit/test_synteny.py
  • src/tbox_finder/mining/synteny_producer.py
  • src/tbox_finder/mining/synteny.py

Comment thread reports/p3/synteny_false_pass.json
…umbered its source

4 findings (CodeRabbit app 1, CLI 3), all real, 2 major. Counts across three rounds: 20, 7, 4.

MAJOR - the strict 5'UTR arm reported 9,087 rows against the wide arm's 9,065.
Impossible for a subset, and the giveaway that the two arms were INDEPENDENT DRAWS each
capped at n_per_host rather than a population and its filter. The strict arm is now derived
from the wide arm's own sample (strict_subsample), so the nesting holds by construction:
8,662 <= 9,090, rates 4.96% wide against 0.29% strict. The report's wording had claimed the
nesting all along.

MAJOR - two units in one report, unlabelled.
The per-clade block sums to 941 CANDIDATES (546 + 96 + 299) while the distance block reported
554 STRAND EVALUATIONS - a candidate passing on both strands contributes two. Not a
discrepancy, but a reader subtracting one from the other would conclude something was
missing. Both blocks now carry an explicit unit and the report states 554 = 546 + 8.

A THIRD ARTIFACT-PINNING TEST WENT GREEN UNDER SABOTAGE.
test_the_utr_arm_is_reported_under_BOTH_readings reads the committed report, so breaking the
derivation could not reach it - the same species as rounds 1 and 2. The filter is now a named
function tested directly for the subset property, the identity of what it removes, and the
all-D4 edge case. Three occurrences in one step: a test that reads a committed artifact
validates the artifact, never the code that made it.

MINOR - test_the_control_is_powered_and_says_why asserted powered and margin but not verdict,
the field a reader consumes and the one that would go stale silently.
MINOR - a decoys parquet missing its accession column raised a non-ProducerError, aborting the
whole sweep AFTER every arm had been computed.

A CLI ROUND THAT READS main -> main IS AN ABSENCE, NOT A PASS.
Round 3's first CLI invocation ran from the main checkout (the backgrounded cd did not apply)
and reported "nothing to review". Re-run from the worktree before the round was counted.

Validation
  full unit+golden 4,635 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  33 sabotages / 33 RED against their NAMED test
  both reports re-derive identically from the final code apart from provenance
  ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@reports/p3/synteny_false_pass.json`:
- Around line 75-79: Reconcile the positive-context control metric in the
report: regenerate it so the stored counts, pass rate, objective, and
control.margin all derive from the same source data. Update the report’s
objective and margin if the 99.414% count-derived rate is correct, or correct
the counts and rate consistently if 99.35% is required.
- Around line 22-30: The strict-arm report must not claim to use the same
sampled windows when false_pass_report independently selects spans with
rng.choice(lengths). Reuse the wide-arm (CDS, span, window) records when
constructing the strict arm, then regenerate the report; alternatively, if
independent window sampling is intentional, revise the note to state that only
the nested CDS population is shared.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 11049879-fa3a-4744-97b7-bd9059fe006a

📥 Commits

Reviewing files that changed from the base of the PR and between b61fd3a and c7beb0a.

⛔ Files ignored due to path filters (1)
  • analyses/phase3_log.qmd is excluded by !**/*.qmd
📒 Files selected for processing (4)
  • reports/p3/synteny_exclusion_diagnostic.json
  • reports/p3/synteny_false_pass.json
  • src/tbox_finder/mining/synteny_producer.py
  • tests/unit/test_synteny_producer.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • reports/p3/synteny_exclusion_diagnostic.json
  • tests/unit/test_synteny_producer.py
  • src/tbox_finder/mining/synteny_producer.py

Comment thread reports/p3/synteny_false_pass.json
Comment thread reports/p3/synteny_false_pass.json
…" was still not true

2 findings (CodeRabbit app), both real, both major. Counts across four rounds: 20, 7, 4, 2.

MAJOR - the strict 5'UTR arm still drew its own spans.
Round 3 made its CDS population a subset of the wide arm's, but each arm still called
rng.choice(lengths) independently, so the windows were never the same windows - only the genes
were. utr_arm_windows now returns (wide, strict) from ONE draw, with strict a sublist of wide,
and the note says what the code does. Final rates: wide 4.74% over 9,097 windows, strict 0.35%
over 8,693.

MAJOR - the PR description drifted from the artifact a second time.
It quoted a 99.35% positive-context rate; the report's own counts give 99.414%. Both drifts in
this step came from hand-copying a number a later regeneration moved, so the reconciliation is
now asserted: test_the_reported_rate_matches_its_own_counts recomputes every arm's rate and
the control margin from the counts beside them.

A FOURTH ARTIFACT-PINNING TEST WENT GREEN UNDER SABOTAGE.
strict_subsample was already tested directly, but the sabotage targeted the CALL SITE, which
no test could see because the only assertion on the wide<->strict relation read the committed
report. utr_arm_windows now owns that relation and is tested for it by identity, with a
positive control and a span-count refusal. Four occurrences in one step; the lesson is not
"add another artifact assertion" but "if the property is a relation, give the relation a name".

Validation
  full unit+golden 4,637 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  34 sabotages / 34 RED against their NAMED test
  ruff 0.15.15 + black 25.11.0 clean

The CLI's round-4 invocation returned "Review limit reached - wait 18 minutes". That is an
ABSENCE, not a pass; it is being waited out rather than escalated.

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/unit/test_synteny_producer.py`:
- Around line 772-780: Update the strict-arm assertions in
test_an_all_non_d4_sample_keeps_every_window and the preceding strict-window
test to validate object identity rather than tuple value equality: use is-based
checks for each corresponding window, while preserving the existing ordering and
coverage expectations.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: fdae0a46-b98a-4d3f-b026-f61c3e98580d

📥 Commits

Reviewing files that changed from the base of the PR and between c7beb0a and b6234ec.

⛔ Files ignored due to path filters (1)
  • analyses/phase3_log.qmd is excluded by !**/*.qmd
📒 Files selected for processing (4)
  • reports/p3/synteny_exclusion_diagnostic.json
  • reports/p3/synteny_false_pass.json
  • src/tbox_finder/mining/synteny_producer.py
  • tests/unit/test_synteny_producer.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • reports/p3/synteny_false_pass.json
  • reports/p3/synteny_exclusion_diagnostic.json
  • src/tbox_finder/mining/synteny_producer.py

Comment thread tests/unit/test_synteny_producer.py Outdated
bioedcam added 17 commits August 6, 2026 22:38
…t tuple equality

1 finding (CodeRabbit app), real, minor. Counts across five rounds: 20, 7, 4, 2, 1.

MINOR - value equality is not the contract.
The round-4 test asserted strict == [wide[1], wide[3]] and all(w in wide for w in strict), but
windows are plain tuples, so a producer that draws a SECOND time and lands on the same span
satisfies value equality while breaking the one-draw contract the test exists to pin. The
assertions are now `is`.

AND THE FIRST SABOTAGE OF THAT FIX WAS ITSELF A NO-OP.
Replacing the returned windows with tuple(w) left the test green, because CPython returns the
SAME object for tuple(t) when t is already a tuple. Checked by execution (tuple(t) is t -> True,
tuple(list(t)) is t -> False) and re-run with a genuinely reconstructing sabotage, which bites.
A sabotage that does not mutate proves nothing about the test.

Validation
  full unit+golden 4,640 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  35 sabotages / 35 RED against their NAMED test
  ruff 0.15.15 + black 25.11.0 clean; no report regenerated (test-only change)

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
… a genomic span, not a coding length

8 CLI findings, 7 real, 2 major (one refuted). Counts across six rounds: 20, 7, 4, 2, 1, 8.
The spike is the CLI's first clean pass since round 3 - its round-4 invocation returned
"Review limit reached", an ABSENCE that was waited out rather than counted.

MAJOR - CdsFeature.length_bp is end - start + 1 over the SPAN.
A frameshifted CDS whose segments sit far apart reads as long even when it codes for little:
measured, a two-segment CDS spanning 901 bp carrying 152 coding nucleotides. There are 273
multi-segment CDS in this corpus and PRD 7.1's tandem loci are exactly where the carve-out has
to fire. gff3.CdsFeature.coding_length_bp (summed segments) now drives the test. NO candidate
verdict changed here, so this is latent correctness the new test pins.

MAJOR - REFUTED. "The sort tiebreak can raise TypeError when feature_id is None."
It cannot: gff3.CdsFeature.feature_id is annotated str and is always populated. The finding
reasoned from DownstreamGene.feature_id: str | None - a different type, whose None case is the
no-gene-found result and never reaches a sort. Checked against the declaration.

Also fixed
  the diagnostics branch converted only three exception types while three calls in it raise
  others, so a malformed input aborted with a traceback instead of a refusal
  the tRNA arm's contig extents came from a CDS-ONLY map, so a tRNA on a CDS-free contig
  skipped the minus-strand upper bound and could cut a window past the contig end
  the strict-arm note had two sentences welded together in a COMMITTED artifact
  ProducerError escaped both mining boundaries as an unhandled RuntimeError, bypassing the
  return-code path the callers branch on; the (c) table is now loaded FIRST

Validation
  full unit+golden 4,644 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  37 sabotages / 37 RED against their NAMED test
  both reports regenerated; status counts unchanged (546 / 96 / 299)
  ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…ailure surface

1 CLI finding, real, major. Counts across seven rounds: 20, 7, 4, 2, 1, 8, 1.

MAJOR - the boundary catch was too narrow.
Round 6 translated ProducerError at both mining boundaries, but load_status_map reads
ARBITRARY JSON: a truncated file raises json.JSONDecodeError, a non-object root raises
AttributeError, a malformed row raises KeyError/TypeError. Every one is an input fault that
bypassed the documented refusal path and would have surfaced as an unhandled traceback on the
leg that decides what gets mined. The catch now covers all of them and the test is
parametrized over four distinct fault shapes rather than the one the previous round happened
to construct.

Validation
  full unit+golden 4,648 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  38 sabotages / 38 RED against their NAMED test
  ruff 0.15.15 + black 25.11.0 clean; no report regenerated (error-path change only)

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…he READ path, and a fifth vacuous test

4 CLI findings, all real, 1 major. Counts across eight rounds: 20, 7, 4, 2, 1, 8, 1, 4.

MAJOR - duplicate candidate_id collapsed at read time.
build_status_table refuses a repeated candidate_id at WRITE time, but load_status_map rebuilt
the map with a dict comprehension, which collapses a duplicate to its LAST occurrence - and
the status<->rows cross-check then compared two maps built the same wrong way, so it agreed
with itself. Two rows saying passed and failed for one candidate would have picked one
silently. A table can reach the reader without having been written by the writer.

THE FIFTH VACUOUS TEST OF THIS STEP, AND ALL FIVE WERE MINE.
test_provenance_never_records_an_absolute_local_path iterates provenance["inputs"], and both
committed reports carry "inputs": {} - so the loop never executed and "outputs" was never
inspected. It now asserts the evidence exists somewhere before checking its shape. Tally of
vacuous species in this step: artifact-pinning assertion (x4), empty-collection loop (x1).

MINOR - pyarrow.lib.ArrowIOError subclasses OSError, so a present-but-unreadable parquet
aborted false_pass_report before it wrote anything.
MINOR - _annotated_set carried a second copy of the accession->filename contract, which would
undercount every host - looking like a smaller corpus rather than a bug - the day
destination_name changed.

Validation
  full unit+golden 4,650 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  40 sabotages / 40 RED against their NAMED test
  ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
… by windows the rule was never asked about

3 CLI findings, all real, 1 major. Counts across nine rounds: 20, 7, 4, 2, 1, 8, 1, 4, 3.
The GitHub app has been silent since b6234ec (five heads), so from round 5 on the CLI has
carried the gate alone - the documented 5.1 fallback. Its round-9 invocation first returned
"Review limit reached"; waiting it out cost nothing and spent no Greptile credit.

MAJOR - _arm computed passed / (passed + failed + unavailable).
A window whose (c) is unavailable is not one where the rule declined to false-pass; it is one
where the rule never ran, and folding it in dilutes the headline by exactly the annotation
gaps the OTHER diagnostic exists to report. Every arm now carries n, n_decided,
n_unavailable and an explicit false_pass_rate_denominator. Corrected rates: random leaders
1.89%, 5'UTR wide 5.00% / strict 0.37%, tRNA-adjacent 2.76%, positive-context 99.52%
(margin 0.976, still powered).

MINOR - the re-anchor could move behind the element 3' end. The fix was right; my first test
for it was not - it picked a target where clamped and unclamped BOTH fail, so the sabotage
stayed green. Measured at the boundary: clamped, a target at 1490 bp is 490 away and passes;
unclamped (anchor at the ORF's end, 980) the same target is 510 away and fails. The clamp is
recall-favouring here and the test now pins both directions. A test that asserts the wrong
direction of a correct fix is indistinguishable from no test.

MINOR - under the `both` policy the exclusion reason could come from a strand whose own status
was passed or failed.

Validation
  full unit+golden 4,657 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  42 sabotages / 42 RED against their NAMED test
  candidate verdicts unchanged (546 / 96 / 299); the clamp moved 0 of them on this corpus
  ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…shed a PASS rate under a false_pass_rate key

6 CLI findings, all real, 1 major. Counts across ten rounds: 20, 7, 4, 2, 1, 8, 1, 4, 3, 6.

MAJOR - the control arm's 99.52% is a pass rate, and it was keyed false_pass_rate.
Windows drawn upstream of a CDS already in a D4 class are SUPPOSED to pass, so that number is
the control working - but under that key, any consumer reading every false_pass_rate in the
file learns that criterion (c) false-passes almost always. The control now emits pass_rate +
pass_rate_denominator; the decoy arms keep theirs.

THE SIXTH ARTIFACT-PINNING MISS, AND THE SHARPEST.
The sabotage for that rename was .get() instead of .pop() - COPY the key rather than move it -
and the committed-report assertion stayed green, because the report it reads was written by
the fixed code. The rename is now as_control_arm, tested for the three things that matter: the
old key is removed (not duplicated), the counts survive, the source arm is not mutated. Six
occurrences in one step, all the same shape: an assertion over a committed artifact cannot see
a producer that writes the artifact correctly for the wrong reason.

MINOR - the duplicate-key rule, a third time: load_clades collapsed a repeated
assembly_accession, and a repeat with a different phylum would silently reassign every
candidate on that host - the per-clade exclusion rate being the headline of the diagnostic.
Verified against the real table: 2,500 assemblies still load.
MINOR - the status table was parsed twice in the diagnostics branch (validate one snapshot,
report from another); now parsed once via validate_status_payload.
MINOR - a row missing candidate_id or status raised a bare KeyError.
MINOR - the re-anchor clamp compared against the fixed three_prime rather than the CURRENT
anchor, so with >1 hop it could not catch a move behind an already-advanced anchor.

Validation
  full unit+golden 4,663 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  44 sabotages / 44 RED against their NAMED test
  candidate verdicts unchanged (546 / 96 / 299); ruff 0.15.15 + black 25.11.0 clean
  two clade-join tests skip here (no pyarrow); both verified by direct execution in
  tbox-finder-data, and CI installs it

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…sed the symbol route it would have fed

6 CLI findings, all real, 1 major. Counts across eleven rounds: 20, 7, 4, 2, 1, 8, 1, 4, 3, 6, 6.

MAJOR - Name=alaS on a hypothetical protein blocked the symbol fallback.
gene_identity_text collects Name and gene alongside product, and a symbol like alaS is neither
boilerplate nor identifier-shaped - so informative went non-empty and the classifier returned
"no D4 class" BEFORE reaching the symbol route that exists for exactly that record.
Reproduced before fixing (-> None, expected aars). A single <=8-character whitespace-free
token is now treated as a name, not a description, with a positive control (DNA gyrase subunit
A + gyrA must still decide None, or a filter that swallowed descriptions would pass alone).

MINOR - the class-II overlap bound was off by one. The element's 5' edge is locus_start + 1 in
1-based coordinates, so a CDS starting on it is -(span - 1) away; >= -span admitted one
coordinate before the element began, on both strands.
MINOR - the overlap allowance leaked past the carve-out. D4's allowance is about the ELEMENT;
once re-anchored, a CDS starting behind that anchor is not downstream of it, and criterion_c
would read the negative distance as in-window.
MINOR - the fail-closed supply derivation could CRASH on a malformed report root; a derivation
that raises is not fail-closed. status_counts values are now type-checked too.
MINOR - a test guarded n rather than n_decided, so an arm that decided nothing raised
TypeError instead of failing an assertion.

Validation
  full unit+golden 4,667 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  48 sabotages / 48 RED against their NAMED test
  candidate verdicts unchanged (546 / 96 / 299) - all three boundary corrections moved 0 of
  them on this corpus, so all three are latent correctness the new tests pin
  ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…d have aborted all 941 verdicts

3 CLI findings, all real, 1 major. Counts across twelve rounds:
20, 7, 4, 2, 1, 8, 1, 4, 3, 6, 6, 3. The CLI's first round-12 invocation returned "Review
limit reached" (its third of this PR); waited out, no Greptile credit spent.

MAJOR - HostAnnotation.__init__ read and parsed the GFF unguarded.
A truncated, undeclared or unreadable file took the whole run down instead of making THAT host
unavailable - the defect -c-i already recorded, in a new place. It is a per-host
unavailability, which ADR-0005 D14 spares, and it now carries its OWN exclusion reason
(host_annotation_unreadable) rather than folding into host_unannotated: a corrupt corpus and a
missing one produce an identical per-clade exclusion rate and must not be indistinguishable in
the diagnostic. Measured: 0 hosts unreadable, so counts are unchanged - the value is that a
future corrupt file costs one host, not the round.

MINOR - the control margin subtracted two rates without checking either for None, so an arm
that decided nothing raised TypeError instead of leaving the control honestly ungraded.
MINOR - the carve-out docstring stated two rules that read as contradictory for a short but
judgeable ORF; the length test is about whether the call is trustworthy, not what it says.

Validation
  full unit+golden 4,671 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  50 sabotages / 50 RED against their NAMED test
  candidate verdicts unchanged (546 / 96 / 299); ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…the wrong thing and would have discarded what it protected

4 CLI findings, all real, 2 major. Counts across thirteen rounds:
20, 7, 4, 2, 1, 8, 1, 4, 3, 6, 6, 3, 4.

MAJOR - the backward-CDS guard keyed on n_intervening, not on the anchor moving.
Round 11 added "after a carve-out, a CDS starting behind the anchor is not downstream". But
round 10's clamp can leave the anchor EXACTLY at the element's 3' end after a hop, and the
guard then rejected the element's own legitimate class-II overlap - a gene criterion (c) is
supposed to see. Two consecutive rounds' fixes interacting; neither was wrong alone.

MAJOR - the tRNA arm re-read and re-parsed every host's GFF.
HostAnnotation exists to parse each host ONCE - its docstring says that cache is why the round
takes seconds - and the tRNA arm went around it, taking a second snapshot of a file the CDS
arm had already committed to. Both arms now read one parse; diagnostics run in 28 s.

MINOR - a zero-length element inverted the overlap bound: -(span - 1) is +1 at span 0, so
distance < 1 discarded the distance == 0 case, D4's class-II abutting locus precisely.
Reproduced before fixing (span=0 -> [], span=1 -> [0]); floored at 0.
MINOR - the P2 path wrapped malformed status tables in a bare ValueError while P3 used its own
RemineError; now MineRoundError, which subclasses ValueError so existing assertions hold.

Validation
  full unit+golden 4,675 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  53 sabotages / 53 RED against their NAMED test
  candidate verdicts unchanged (546 / 96 / 299); ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…rding a four-class risk

1 CLI finding, real, minor. Counts across fourteen rounds:
20, 7, 4, 2, 1, 8, 1, 4, 3, 6, 6, 3, 4, 1.

MINOR - test_a_bare_glutamine_amidotransferase_is_NOT_transamidation asserted
!= CLASS_TRANSAMIDATION, but the risk its own docstring names is INFLATING criterion (c) - and
a classifier routing "type 1 glutamine amidotransferase" into aa_biosynthesis instead would
inflate it exactly as much while satisfying the assertion. Now `not in PASSING_CLASSES`, and
sabotaged by a CROSS-CLASS mis-route (adding amidotransferase to the biosynthesis pattern)
rather than by breaking the transamidation rule - the shape the old assertion was blind to.

Validation
  full unit+golden 4,675 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  54 sabotages / 54 RED against their NAMED test
  no source change, so no report regenerated; ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…nothing read, and two reports that could land out of step

6 CLI findings, all real, 1 major (maintainability). Counts across fifteen rounds:
20, 7, 4, 2, 1, 8, 1, 4, 3, 6, 6, 3, 4, 1, 6.

THE COUNT BOUNCED BACK, BUT THE KIND DID NOT.
The last finding that could change a verdict was round 13's guard interaction; rounds 14-15
are API hygiene and defensive hardening - a required parameter with no effect, write-ordering
of two reports, refusals for inputs the corpus does not contain. Recorded as a convergence
signal even though the count is not monotone.

MAJOR - exclusion_report took annotation_dir and genome_dir as REQUIRED keyword arguments and
read neither; six test call sites passed "unused" for both, which is a test asserting
something false about the API. Removed.

MINOR - the false-pass report was written BEFORE the exclusion report was built, so a failure
in between left a committed false-pass report beside a stale exclusion report - and
test_both_reports_describe_the_same_run_config asserts the pair describes one run. Both are
now built before either is written.
MINOR - an empty candidate list left lengths empty and every rng.choice in the three control
arms raised IndexError, uncaught by the CLI.
MINOR - a scalar manifest row raised TypeError from dict(row) rather than a ProducerError.
MINOR - the strict/wide comparison guarded n rather than n_decided, where None <= float raises.
MINOR - two parametrized test labels named exceptions the producer no longer raises.

Validation
  full unit+golden 4,679 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  56 sabotages / 56 RED against their NAMED test
  both reports re-derived; candidate verdicts unchanged (546 / 96 / 299)
  ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…o sibling reads

2 CLI findings, both real, 1 major. Counts across sixteen rounds:
20, 7, 4, 2, 1, 8, 1, 4, 3, 6, 6, 3, 4, 1, 6, 2.

MAJOR - HostAnnotation reads a GFF AND a genome FASTA; round 12 wrapped the first and left the
second bare. One empty or unreadable .fna - or a header with no record id - still took the
whole round down instead of costing its own host. Fixed with its own reason
(genome_fasta_unreadable), kept distinct from genome_fasta_absent for the same reason the
annotation pair is split: a corrupt file and a missing one must not be indistinguishable in
the diagnostic. Fixing one of two identical reads and calling the class of defect closed is
the mistake worth naming.

MINOR - the provenance guard used Path(key).is_absolute(), which selects PurePosixPath
semantics on a POSIX runner, so C:\Users\alice\report.json reads as RELATIVE and a
home-directory leak slips past the assertion written to catch it. Both PurePosixPath and
PureWindowsPath are checked now.

Validation
  full unit+golden 4,687 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  58 sabotages / 58 RED against their NAMED test
  candidate verdicts unchanged (546 / 96 / 299); ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
A fourth hand-copied number drifted in this step. The round-16 stanza said 4,687 passed; the
run printed 4,683. Four occurrences total - the distance percentiles (twice), the
positive-control rate, and now a pass count - every one from transcribing a number instead of
reading it back from the thing that produced it.

The committed REPORTS are protected by test_the_reported_rate_matches_its_own_counts, which
recomputes every rate from the counts beside it. The dev-log PROSE is not, and that is the
remaining hole.

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…the cache the module is built around

2 CLI findings, both real, 1 major. Counts across seventeen rounds:
20, 7, 4, 2, 1, 8, 1, 4, 3, 6, 6, 3, 4, 1, 6, 2, 2.

MAJOR - run-shard sliced the manifest with candidates[i::n], which INTERLEAVES.
One host's candidates scattered across every shard, and _host_cache is created per run_shard
call, so each shard re-parsed almost every host's GFF - turning one parse per host into
n_shards parses per host, defeating the exact cache whose docstring says it is why the round
takes seconds. shard_by_host now assigns whole hosts round-robin over their sorted order:
deterministic and independent of manifest order.

AND THE SHARD -> MERGE PATH IS NOW PROVEN END-TO-END, WHICH IT HAD NEVER BEEN.
Three shards run separately and merged reproduce the single-shard table exactly - 941
verdicts, 546 / 96 / 299, status maps identical. The contract had only ever been exercised on
synthetic two-row tables.

MINOR - the distance series ignored strand_policy: it collected from any strand detail whose
own status was passed, without checking whether the policy folded that strand into the
candidate's verdict. Under plus/minus the other strand could contribute a distance describing
a decision the round never made. The committed reports run under `both`, so no published
number moves.

Validation
  full unit+golden 4,687 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  59 sabotages / 59 RED against their NAMED test, including a control proving a strided slice
  does scatter hosts, so the new assertion has teeth
  candidate verdicts unchanged (546 / 96 / 299); ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…ompound

The stanza was lost from its own commit: the shell line began `cd <worktree> && cat >> …`, the
cd failed because the session was already in the worktree, and && short-circuited the append -
while the newline-separated git commit that followed ran anyway. Third occurrence in this step.
A compound whose first element is a redundant cd silently drops everything after it, and the
commit still succeeds, so nothing looks wrong.

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
… one of two identical things"

3 CLI findings, all real, all minor. Counts across eighteen rounds:
20, 7, 4, 2, 1, 8, 1, 4, 3, 6, 6, 3, 4, 1, 6, 2, 2, 3.

All three are the shape round 16 named:
  a test built its real annotation filenames through annotation_fetch.destination_name and
  then hardcoded "GCA_000296795.1.gff.gz.part" beside them - the second copy of the
  accession->filename contract that function's docstring warns about
  round 16 replaced Path(key).is_absolute() with a dual PurePosixPath/PureWindowsPath check
  for inputs/outputs and left the external_inputs name on the single-OS test
  decision["max_bp"] was guarded against None and decision["p99_bp"] directly beneath it was
  not, though the same schema reports None for both on an empty series

Three rounds running (16, 17, 18) whose findings are "you fixed one of two identical things".
Stated as a rule: when a fix lands, grep for the other call site before declaring the class
closed.

Validation
  full unit+golden 4,687 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  60 sabotages / 60 RED against their NAMED test
  test-only change, so no report regenerated; ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
…e is met

Counts across nineteen rounds: 20, 7, 4, 2, 1, 8, 1, 4, 3, 6, 6, 3, 4, 1, 6, 2, 2, 3, 0.
83 findings, 82 real, 30 major.

The CLI reported "No new findings - 3 previous findings". Those three are round 18's and each
was VERIFIED present in the tree rather than assumed: the p99_bp None guard, the dual
PurePosixPath/PureWindowsPath check on entry["name"], and the .part name derived from
destination_name. A stored finding from a prior run is not an outstanding one.

Round 19 was the CLI's fourth Fair-Usage stall (after 4, 9, 12). Every one was treated as an
absence, not a pass, and waited out; no Greptile credit was spent across the whole step
(12/16 remaining). The GitHub app went silent after b6234ec, so rounds 5-19 were carried by
the CLI alone - the documented 5.1 fallback.

Final state
  941/941: 546 passed / 96 failed / 299 unavailable
  false-pass (decided denominator): random leaders 1.86%, 5'UTR 4.90% wide / 0.30% strict,
  tRNA-adjacent 2.82%, positive control 99.43% pass_rate (margin 0.976, powered)
  per-clade exclusion 31.8%, five phyla at 100%
  full unit+golden 4,687 passed / 13 failed vs the matched baseline at 95d0149 (4,492 / 13)
  - failure sets identical, comm empty both ways
  60 sabotages / 60 RED against their NAMED test
  both reports re-derive identically from the final code apart from provenance
  CI green on all four jobs; ruff 0.15.15 + black 25.11.0 clean

Claude-Session: https://claude.ai/code/session_017VRtJL7TgZioPxpCUuENmM
@bioedca
bioedca merged commit e228717 into main Aug 7, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants