You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The codebase has two record types in the classification result. One is guarded by a constructor with required, checked slots. The other is a bare dict literal with an implicit schema and a silently-defaulting required field.
Field entry — {value, status, evidence} — is locked down. models.build_field_entry is "the single place that assembles the output shape"; _assert_coherent raises if value and status contradict; set_field raises on an unknown field or an invalid status. A sentinel cannot reach the value slot.
Claim — the evidence dict appended to field_evidence[fld] — has no constructor. Its schema exists only in the consumers that read it:
key
read by
if omitted
value / status
_claim_declaration (rule_engine.py:186)
claim silently ignored during resolution
tier
evaluate_claims (rule_engine.py:263-264), via .get("tier", 0)
silently becomes tier 0
rule_id
rule_engine.py:138,151, via e["rule_id"]
KeyError, loudly
reason
display only
nothing
The failure modes are inconsistent, and the one that silently changes the answer — tier — is the one with a defensive default.
Why it doesn't bite in the rules path
_apply_rule (rule_engine.py:583) builds every rule-derived claim from a single evidence_entry literal carrying rule_id, reason, tier, then spreads it into a value-claim or a status-claim. One construction site. Tier is never forgotten, and the loader already enforces value-xor-status per field (rule_loader.py:349 raises when a field appears in both then and then.status).
_apply_rule is already the API this issue asks for. It just isn't exposed.
Where it does bite
Content classifiers hand-roll claims outside that constructor. In header_classifier.py there are 19 claim literals, of which exactly 1 sets a tier (added in #148/#151). rule_engine.py:672 (infer_assay_type) omits it too.
Worse, those sites maintain two independent sources of truth — the claim list and the field's value — and nothing checks they agree:
result.field_evidence["data_type"].append({... "value": "pangenome.reference" ...})
result.set_field("data_type", "pangenome.reference") # nothing verifies these match
This is not hypothetical. In #151 the first draft omitted tier from the appended rGFA claim. evaluate_claims would have defaulted it to 0, lost to the tier-1 pangenome claim via higher_specificity_override, and resolved to pangenome — contradicting the pangenome.reference written by set_field. It only stayed hidden because nothing re-runs resolution on that path. The evidence would have contradicted the answer, silently.
Proposed API
One method on ExtendedClassificationResult:
defadd_claim(self, fld, *, rule_id, reason, tier, value=None, status=None):
"""Append a claim and re-resolve the field from all its claims."""self._require_field(fld)
# value xor status — the same invariant rule_loader enforces for rulesclaim= {"rule_id": rule_id, "reason": reason, "tier": tier}
claim["value"ifvalueisnotNoneelse"status"] =valueorstatusself.field_evidence[fld].append(claim)
ev=evaluate_claims(self.field_evidence[fld])
self.set_field(fld, ev["value"], ev["status"])
What this buys:
tier cannot be forgotten — required keyword slot.
Value cannot diverge from evidence — it is derived from the claim list, not written alongside it. One source of truth.
Conflict detection everywhere, free — evaluate_claims runs on every addition, so a competing same-tier claim raises a conflict instead of being silently overwritten by whichever line runs last.
evaluate_claims can stop being defensive — .get("tier", 0) becomes claim["tier"], turning a silent wrong answer into a loud KeyError.
_apply_rule collapses onto it, so rules and content classifiers share one path.
Migration
18 wholesale field_evidence[...] = [...] assignments in header_classifier.py (BAM contig detection, VCF, FASTQ, FASTA, BED) each discard the tier-1/2 claims the engine produced and replace them with a single untiered entry. Each becomes an add_claim call.
rule_engine.py:668 is a different smell — it filters a stale not_classified placeholder out of the list rather than clobbering it. Worth revisiting once add_claim re-resolves on every addition, since the placeholder would no longer need removing by hand.
The one already-correct site is header_classifier.py:271 (extend), plus the engine's own appends.
Notes on scope
FASTA's logic (contig counting, >= 20 matching chromosomes to call a reference, assembler/transcript pattern buckets) will not reduce to a declarative YAML when: predicate. That is fine and is the point: add_claim lets imperative classifiers emit claims without becoming rules. A gfa_header YAML scope is optional and secondary.
Migrating one classifier at a time is safe — add_claim and the existing assignment can coexist during the transition. But the golden fixture (tests/fixtures/golden/expected_output.json) pins evidence for every file type, so each migration will show its provenance change there. That is the review signal, not a problem.
Expect real output changes: fields that currently show one hand-rolled entry will show the full derivation chain. Detect pangenome reference graphs from rGFA content (#148) #151 already did this for GFA — data_type evidence went from [rgfa_stable_rank_reference] to [tier=1 pangenome_graph, tier=3 rgfa_stable_rank_reference], matching what the engine emits for the -mc- case.
Tasks
Add ExtendedClassificationResult.add_claim with required rule_id/reason/tier and value-xor-status enforcement.
Re-express _apply_rule in terms of it (or share the claim constructor).
Migrate the 18 header_classifier.py sites, one classifier per commit, regenerating the golden each time.
Add tier to rule_engine.py:672 (infer_assay_type).
Tighten evaluate_claims to claim["tier"] once no caller can omit it.
Revisit the rule_engine.py:668 placeholder filter.
Refs: #148/#151 (GFA content check — the near-miss that surfaced this, and the one site already appending with a tier), #116/#136 (status/value split, which fixed the field entry half of this problem), #56 (claim/evidence model).
The defect
The codebase has two record types in the classification result. One is guarded by a constructor with required, checked slots. The other is a bare dict literal with an implicit schema and a silently-defaulting required field.
Field entry —
{value, status, evidence}— is locked down.models.build_field_entryis "the single place that assembles the output shape";_assert_coherentraises ifvalueandstatuscontradict;set_fieldraises on an unknown field or an invalid status. A sentinel cannot reach thevalueslot.Claim — the evidence dict appended to
field_evidence[fld]— has no constructor. Its schema exists only in the consumers that read it:value/status_claim_declaration(rule_engine.py:186)tierevaluate_claims(rule_engine.py:263-264), via.get("tier", 0)rule_ide["rule_id"]KeyError, loudlyreasonThe failure modes are inconsistent, and the one that silently changes the answer —
tier— is the one with a defensive default.Why it doesn't bite in the rules path
_apply_rule(rule_engine.py:583) builds every rule-derived claim from a singleevidence_entryliteral carryingrule_id,reason,tier, then spreads it into a value-claim or a status-claim. One construction site. Tier is never forgotten, and the loader already enforces value-xor-status per field (rule_loader.py:349 raises when a field appears in boththenandthen.status)._apply_ruleis already the API this issue asks for. It just isn't exposed.Where it does bite
Content classifiers hand-roll claims outside that constructor. In
header_classifier.pythere are 19 claim literals, of which exactly 1 sets atier(added in #148/#151).rule_engine.py:672(infer_assay_type) omits it too.Worse, those sites maintain two independent sources of truth — the claim list and the field's value — and nothing checks they agree:
This is not hypothetical. In #151 the first draft omitted
tierfrom the appended rGFA claim.evaluate_claimswould have defaulted it to 0, lost to the tier-1pangenomeclaim viahigher_specificity_override, and resolved topangenome— contradicting thepangenome.referencewritten byset_field. It only stayed hidden because nothing re-runs resolution on that path. The evidence would have contradicted the answer, silently.Proposed API
One method on
ExtendedClassificationResult:What this buys:
tiercannot be forgotten — required keyword slot.evaluate_claimsruns on every addition, so a competing same-tier claim raises a conflict instead of being silently overwritten by whichever line runs last.evaluate_claimscan stop being defensive —.get("tier", 0)becomesclaim["tier"], turning a silent wrong answer into a loudKeyError._apply_rulecollapses onto it, so rules and content classifiers share one path.Migration
18 wholesale
field_evidence[...] = [...]assignments inheader_classifier.py(BAM contig detection, VCF, FASTQ, FASTA, BED) each discard the tier-1/2 claims the engine produced and replace them with a single untiered entry. Each becomes anadd_claimcall.rule_engine.py:668is a different smell — it filters a stalenot_classifiedplaceholder out of the list rather than clobbering it. Worth revisiting onceadd_claimre-resolves on every addition, since the placeholder would no longer need removing by hand.The one already-correct site is
header_classifier.py:271(extend), plus the engine's own appends.Notes on scope
>= 20matching chromosomes to call a reference, assembler/transcript pattern buckets) will not reduce to a declarative YAMLwhen:predicate. That is fine and is the point:add_claimlets imperative classifiers emit claims without becoming rules. Agfa_headerYAML scope is optional and secondary.add_claimand the existing assignment can coexist during the transition. But the golden fixture (tests/fixtures/golden/expected_output.json) pins evidence for every file type, so each migration will show its provenance change there. That is the review signal, not a problem.data_typeevidence went from[rgfa_stable_rank_reference]to[tier=1 pangenome_graph, tier=3 rgfa_stable_rank_reference], matching what the engine emits for the-mc-case.Tasks
ExtendedClassificationResult.add_claimwith requiredrule_id/reason/tierand value-xor-status enforcement._apply_rulein terms of it (or share the claim constructor).header_classifier.pysites, one classifier per commit, regenerating the golden each time.tiertorule_engine.py:672(infer_assay_type).evaluate_claimstoclaim["tier"]once no caller can omit it.rule_engine.py:668placeholder filter.Refs: #148/#151 (GFA content check — the near-miss that surfaced this, and the one site already appending with a tier), #116/#136 (status/value split, which fixed the field entry half of this problem), #56 (claim/evidence model).