Add safeguards for broken ontology adapters in reachable_from

[cmungall]

Summary

Add two new exception types and validation logic to detect and fail loudly when a reachable_from dynamic enum is configured with a broken or misconfigured ontology adapter. This prevents silent false negatives where terms are incorrectly rejected as "not in enum" due to adapter defects.

Key Changes

• New exception types (EmptyReachableClosureError, InconsistentReachabilityError):

  • EmptyReachableClosureError: Raised when a source node resolves but its closure is empty, which would silently reject all terms and poison greedy cache expansions
  • InconsistentReachabilityError: Raised when an adapter's ancestor/descendant directions disagree by CURIE (e.g., OLS4 MONDO defect dismech#7012), causing silent false negatives in ancestor-based membership checks

• Round-trip consistency probing in DynamicEnumPlugin:

  • _reachability_inconsistency_witness(): Samples a few closure members and verifies they report the source node in the reverse direction; caches results per (source_node, predicates, direction)
  • _sample_closure() and _graph_closure(): Helper methods to safely query adapter closures with fallback to OLS REST endpoints
  • Probing is triggered before returning a negative in _is_value_in_reachable_from() to catch silent false negatives

• Empty closure detection in _expand_reachable_from():

  • Tracks whether any source node resolved
  • Raises EmptyReachableClosureError if at least one source resolved but the expansion is empty
  • Prevents empty-but-complete cache entries from poisoning later runs

• CLI exit code for misconfiguration:

  • New exit code 3 (EXIT_ONTOLOGY_MISCONFIGURED) for UnreliableReachabilityError
  • Distinct from invalid data (1) and service outages (2) to signal a configuration problem

• Comprehensive test coverage:

  • Tests for inconsistent adapter behavior (OLS4 MONDO defect simulation)
  • Tests confirming legitimate negatives (out-of-enum terms, childless leaves) are not flagged
  • Tests for multi-source unions and cache behavior
  • Integration test against live OLS MONDO to guard against regression

Implementation Details

• Probing is deliberately conservative: a genuine out-of-enum term keeps both directions in agreement (absent from both), so correct negatives are never flagged
• Childless leaf sources reach nothing, so nothing round-trips — correct negatives under them stay quiet
• Multi-source unions (leaf + populated branch) still expand non-empty and are unaffected
• The motivating case (dismech#7012) is a CURIE/identifier merge where OLS4 returned MONDO:0000001 under the wrong obo_id AFO_O:0000001, making the term unmatchable by CURIE despite correct hierarchy by IRI
• Recommended fix: configure a local, deterministic adapter (e.g., sqlite:obo:mondo) for affected prefixes

https://claude.ai/code/session_01GJHtqxPh5cdTSyx8uiBAXG

[claude]

Claude finished @cmungall's task in 4m 12s —— View job

---

Review: broken-adapter safeguards for reachable_from

• Read the diff and surrounding code
• Analyze correctness / performance / test coverage
• Post review

Nice, well-motivated piece of work — the round-trip idea is the right invariant for catching a CURIE-level identifier merge, the "answered vs. could-not-answer" distinction in _graph_closure shows the failure mode was thought through, and the docstrings carry real diagnostic value. A few things I'd want addressed before this guards production runs, since both new errors are hard aborts of the whole validation run — a false positive is more expensive than the silent false negative being fixed.

I could not execute the test suite in this job (bash approval), so everything below is from reading the code.

---

High — false-positive risks in the round-trip probe

1. "Answered with an empty set" is treated as proof of inconsistency. plugins/base.py:858-895 deliberately distinguishes "could not answer" (None) from "answered: empty" (set()), but the caller at base.py:811 then treats an empty reverse closure as a confirmed witness. An adapter that merely returns nothing for a direction it doesn't really support — different predicate spelling (is_a vs rdfs:subClassOf), a focus-ontology restriction, a direction implemented as a no-op — will hard-fail an otherwise healthy setup. The OLS descendants()-returns-nothing case is exactly this shape, and it's only rescued because of the special-cased _ols_descendants fallback; any other adapter with the same asymmetry gets no rescue. Suggest only flagging when at least one sampled member produced a non-empty reverse closure (i.e. the reverse direction demonstrably works) before concluding the round trip is broken. Fix this →

2. Sampled closure members are not prefix-filtered. _sample_closure (base.py:818) returns whatever the traversal yields, which for merged/imported ontologies (and for OLS's obo_id-based descendant fallback) can include foreign-prefix terms. The reverse lookup then runs on the source's adapter — for an OLS adapter with focus_ontology, a foreign CURIE will typically come back empty, and combined with issue 1 that raises InconsistentReachabilityError on a perfectly good ontology. The call site at base.py:708 already applies a same-prefix restriction to value; the sample deserves the same. Fix this →

3. First-failure-wins contradicts the stated sampling rationale. The comment at base.py:771-775 says a sample larger than one "guards against an incidental multi-parent term whose sampled edge happens to hold" — but the loop raises on the first member that fails, so a larger sample only makes flagging more likely, never less. If consensus was the intent (and given the blast radius it probably should be), require all (or a majority of) sampled members to fail. Real ontologies do contain odd terms — obsolete/detached nodes, terms whose ancestors are computed over a different graph — and one of those shouldn't abort a whole run.

---

High — performance

4. The probe computes full closures, and now fires on the first legitimate negative. For every sampled member, _graph_closure materializes the member's entire closure (base.py:874-876) rather than early-exiting once source_node is seen. In the healthy case the source node is found almost immediately, so early exit would make the common path essentially free. Three compounding costs:

• traverse_up=True inverts the directions: forward is ancestors(source) (near-root terms), reverse is descendants(member) — i.e. potentially the whole ontology, up to 8 times, over REST for a remote adapter.
• _sample_closure's OLS fallback (base.py:848-855) calls _ols_descendants, which pages through all descendants before the [:8] slice — defeating the "iterate lazily, stop early, probing a huge root is cheap" promise in its own docstring, for precisely the MONDO-root scenario that motivated the PR.
• The trigger is a legitimate negative — one out-of-enum term in a data file now pays for a root-scale crawl on first occurrence.

The per-source memoization bounds this to once per source node per run, which helps, but the first hit can still be minutes. Early exit on finding source_node, and/or probing one hop (children()/parents()) instead of a full closure, would address most of it. Fix this →

---

Medium

5. Same enum, two different verdicts depending on cache strategy. A single childless-leaf source is a quiet False on the progressive path (test_progressive_leaf_source_negative_does_not_flag) but a hard EmptyReachableClosureError under greedy expansion (test_greedy_empty_source_closure_is_not_cached_as_complete — both use TEST:0000004). That's a surprising strategy-dependent behavior change for existing users, and docs/enumerations.md states "Both checks are deliberately conservative and never flag a legitimate config … a childless leaf source … stays a quiet False" — which is true of check 1 but not check 2. Either align the two paths or, at minimum, correct the doc so the leaf-source bullet doesn't read as covering the empty-closure check.

6. EmptyReachableClosureError names the wrong source node. base.py:1150-1153 always reports query.source_nodes[0], but the trigger condition is "some source resolved". With source_nodes: [UNRESOLVABLE:1, REAL:2], the message blames the unresolvable one and suggests the wrong adapter fix. Track which node(s) actually resolved and name those.

7. Traversal logic is now duplicated three ways. _call_graph_traversal, _sample_closure, and _graph_closure each wrap the same "call adapter method, tolerate variants, normalize errors" pattern. The two new ones drop the inspect.signature handling and pass predicates= positionally-by-keyword, so an adapter with a different signature raises TypeError → caught → empty → the guard is silently disabled for that adapter. Fail-safe, but worth a logger.warning rather than logger.debug so a permanently-disabled guard is discoverable.

8. Private-member access across module boundaries. self.ontology._is_ols_adapter(adapter) (base.py:848, 886) reaches into OntologyAccess internals. The pre-existing code in _expand_reachable_from instead calls _ols_descendants unconditionally and lets it self-guard on missing attributes — consider following that or promoting _is_ols_adapter to public.

9. Naming: InconsistentReachabilityError.descendant holds an ancestor when traverse_up=True (the message text handles it correctly, the attribute name doesn't). witness or closure_member would read better, and the tests assert on this attribute so it's cheap to rename now.

---

Test coverage

Solid unit coverage of the offline defect simulation and the three "must stay quiet" cases. Gaps I'd fill:

• No traverse_up=True test for the probe. The forward/reverse direction mapping at base.py:805-806 is the trickiest logic in the PR and a swap would go unnoticed — plus that's the branch with the worst performance profile.
• No CLI test for exit code 3. tests/test_cli.py:117 / :160 establish exactly the pattern (monkeypatched adapter + assert result.exit_code == 2); a sibling test asserting 3 and the new message would lock in _fail_unreliable_reachability.
• No test for _graph_closure returning None (reverse direction unanswerable → must not flag), nor for the answered-empty case in issue 1 — the two branches carrying the false-positive risk are the two untested ones.
• The integration test is a good regression guard, but note it passes both when the guard works and when the guard is silently disabled (issue 7) — a negative-direction assertion would strengthen it.

Nothing security-relevant; no new network surface beyond existing adapter calls, and the double-quoted IRI encoding in _ols_descendants is unchanged.
· branch claude/ltv-issue-7012-ed5bc9

[claude]

Claude finished @cmungall's task in 3m 52s —— View job

---

Re-review after eaf7221 ("Harden reachability integrity guard per PR #69 review")

• Read the updated diff and surrounding code
• Check which prior review points were addressed
• Analyze correctness / performance / test coverage
• Post review

The hardening commit is a real improvement — the consensus rule (_compute_inconsistency_witness returns None the moment any member round-trips, and requires a demonstrably-answerable reverse direction before flagging) is exactly the right shape, and it closes prior points 1, 2, 3, 6, 8 and 9. _reverse_reaches early-exits, the witness rename landed, _is_ols_adapter private access is gone, resolved_sources[0] names a node that actually resolved, and the traverse_up + CLI-exit-3 tests were added. Docs were corrected on the leaf-source/empty-closure distinction.

Two things I think still need to change before merge. I could not run the test suite in this job (bash approval), so this is from reading the code.

---

High — the guard is a no-op on a live OLS adapter, for the exact case that motivated it

_sample_closure (src/linkml_term_validator/plugins/base.py:855-890) now deliberately skips the OLS REST fallback:

the paged OLS descendants crawl is intentionally not used here — … only the native, already bounded traversal is sampled

But for the default direction (traverse_up=False), forward = "descendants" (base.py:834), and this repo's own code documents that OAK's OLS adapter answers descendants() with nothing — that's why _ols_descendants exists (base.py:1213, "Fallback descendant expansion for OAK OLS adapters without descendants()"), why _expand_reachable_from falls back at base.py:1184-1190, and why _reverse_reaches special-cases it at base.py:934-936.

So on ols:mondo: _sample_closure(adapter, "descendants", "MONDO:0000001", …) → [] → answerable = False → witness None → InconsistentReachabilityError can never fire. And it fails silently: the empty result comes from an exception-free traversal, so not even the new logger.warning triggers. The dismech#7012 defect would still have produced silent false negatives through the progressive path.

The new integration assertion doesn't catch this, because it calls _reverse_reaches directly (tests/test_oak_providers_integration.py:130-136) — i.e. it exercises the reverse step while the forward sampling step, which is the one that returns nothing on OLS, is never touched.

Two suggestions, either works:

• Sample one hop instead of a closure — the CURIE-merge signature is visible at the first hop (a direct child also fails to list the root by CURIE), and one hop is cheap on every adapter including OLS's children endpoint.
• Or let _sample_closure consult a bounded _ols_descendants (first page only) when the native traversal yields nothing.

At minimum, _compute_inconsistency_witness should logger.warning when the forward sample is empty and the source node is known to have children, so a silently-disabled guard is discoverable. Fix this →

---

High — EmptyReachableClosureError fires per clause, not per enum, contradicting the docs

docs/enumerations.md says the check "fires only when the entire query expands to nothing". But the raise lives inside _expand_reachable_from (base.py:1205-1208), which is one of several contributors merged by expand_enum:

call site	consequence of an empty reachable_from
base.py:1038	enum with reachable_from + permissible_values / concepts / inherits aborts even though the enum is well-populated (base.py:1044-1072)
base.py:1101 via include:	one empty branch of a union aborts the whole union
base.py:1056 via minus:	a minus clause rooted at a leaf — "subtract nothing", entirely legitimate — hard-aborts with exit code 3

The minus case is the clearest false positive: excluding an empty subtree is a normal, correct configuration, and it now kills the run.

The fix is to move the decision up: have _expand_reachable_from signal "resolved sources but empty" (return value or flag) and let expand_enum raise only when the whole enum comes out empty — which is also what the docs already promise. Fix this →

---

Medium

3. traverse_up + OLS is still a potential root-scale crawl. _reverse_reaches early-exits on the native iteration (good), but when the native traversal yields nothing it calls _ols_descendants (base.py:937-942), which pages through the entire descendant set with no early exit. Under traverse_up=True the reverse direction is descendants and the sampled members are the source's ancestors — i.e. near-root terms — so a single legitimate negative can trigger up to _INCONSISTENCY_SAMPLE_SIZE (8) full paged crawls before the first verdict. The per-source memoization bounds it to once per run, but the first hit can be minutes. Bounding the fallback (first page, or short-circuit as soon as target appears) would fix it.

4. resolved_sources costs a label lookup per source node on every expansion. base.py:1160-1163 calls get_ontology_label(source_node) unconditionally, but the result is only consulted in the not values branch at base.py:1205. Defer the resolution check into that branch so the healthy path pays nothing.

5. Sample cap is applied before prefix filtering. _sample_closure stops at 8 members (base.py:875), and only then does _compute_inconsistency_witness drop foreign-prefix terms (base.py:840-842). For a merged/imported ontology whose traversal happens to yield foreign CURIEs first, all 8 are discarded and the guard silently disables itself. Filtering inside the sampling loop (pass the expected prefix down) makes the sample actually contain 8 usable members.

6. An exception in the reverse traversal skips the OLS fallback. base.py:923-932 returns _REVERSE_UNANSWERABLE on any exception, without trying _ols_descendants — whereas _expand_reachable_from reaches the fallback whenever the native result is falsy. An OLS adapter whose descendants() raises (rather than returning empty) therefore gets no fallback. Fail-safe, but inconsistent with the sibling path.

---

Test coverage

Good additions: _EmptyReverseAdapter covers the answered-empty branch, _InconsistentTraverseUpAdapter locks the forward/reverse direction mapping, and the CLI exit-3 test locks _fail_unreliable_reachability. Remaining gaps map onto the findings above:

• No test that _reachability_inconsistency_witness gets a non-empty forward sample from an adapter whose descendants() returns nothing — the finding-1 shape. An offline stub (descendants() → empty, _ols_descendants-style fallback populated) would pin it.
• No CLI test for exit code 3 via EmptyReachableClosureError — only the unit-level pytest.raises. The inconsistency error has one; the empty-closure error should too, since they share the exit path.
• No test for reachable_from composed with minus: / include: / permissible_values — finding 2 would have shown up immediately.

Nothing security-relevant; no new network surface beyond existing adapter calls.
· branch claude/ltv-issue-7012-ed5bc9

[cmungall]

Thanks — this was a genuinely useful review; the false-positive framing (a hard abort is worse than the silent negative) is exactly right. Addressed in eaf7221.

High — false-positive risks (all fixed, they were the core of the rewrite):

• Bump actions/setup-node from 3 to 6 #1 answered-empty treated as proof. The probe now requires the reverse direction to be demonstrably working — a witness is reported only when at least one sampled member has a non-empty reverse closure that omits the source. An adapter whose reverse direction is a no-op / wrong-predicate / focus-restricted returns empty → UNANSWERABLE → never flagged. New test test_progressive_empty_reverse_closure_does_not_flag.
• Bump actions/setup-python from 5 to 6 #2 no prefix filter. Sampled members are now filtered to the source node's prefix before the reverse lookup.
• Bump astral-sh/setup-uv from 5 to 7 #3 first-failure-wins. Replaced with consensus: any single member that does round-trip returns "consistent" immediately (healthy fast path); a witness is raised only when no member round-trips and ≥1 is answerable-without. A larger sample now makes flagging less likely, matching the stated rationale.

High — performance (#4, fixed): introduced a tri-state _reverse_reaches that early-exits the moment the source is found, so the healthy path is essentially free even on a huge closure; and _sample_closure no longer falls back to the paged _ols_descendants crawl (native, already-bounded traversal only). Your point was well-founded — the integration test's extra probe assertion actually tripped the 5s read-timeout on the root crawl, which I then scoped down.

Medium:

• feat: major documentation restructure and validation enhancements #6 EmptyReachableClosureError now names a source node that actually resolved, not source_nodes[0].
• term cache files should be free of spurious diffs #9 renamed .descendant → .witness (it holds an ancestor under traverse_up).
• feat: add configurable cache strategies for dynamic enums #7 probe failures now log at warning so a silently-disabled guard is discoverable. I did not fully collapse the three traversal helpers — _call_graph_traversal (materialize-all, for expansion), _sample_closure (bounded sample), and _reverse_reaches (early-exit tri-state) have genuinely different semantics now; merging them would re-entangle the early-exit behavior Bump actions/github-script from 6 to 8 #4 needs.
• feat: enable label validation by default #8 dropped the cross-module _is_ols_adapter access; _ols_descendants already self-guards to a no-op on non-OLS adapters.
• Bump actions/checkout from 4 to 5 #5 I took the "correct the doc" path rather than aligning the two code paths, because they answer different questions: the round-trip check is about one value, the empty-expansion check is about the whole query being empty. Docs now scope each accordingly.

Test coverage added: traverse_up inconsistency and healthy cases (locks the forward/reverse mapping), empty-reverse no-flag, a CLI exit-code-3 case, and a live-OLS assertion that the reverse direction resolves the disease root by CURIE (proving the guard is wired, not a no-op).

216 unit tests pass; mypy + ruff clean; OLS integration test passes against the now-fixed API.

Context for reviewers: the motivating OLS defect (EBISPOT/ols4#1334) has since been fixed upstream, so this guard is retained as a general safety net against this class of adapter identifier corruption rather than a fix for a currently-active bug.

---

Generated by Claude Code

[cmungall]

Both High findings were spot-on — fixed in c745f6a.

High #1 — guard could be a silent no-op on live OLS. Correct: _sample_closure had dropped the OLS descendants fallback, and OAK's OLS descendants() returns nothing on some versions, so the forward sample was empty → witness None → guard never fires (I'd only asserted the reverse step, which is why the integration test didn't catch it). Restored a bounded _ols_descendants fallback in _sample_closure (new limit arg → stops paging early; the full root crawl is never done). Prefix filtering now happens inside the sampling loop (your Medium #5), so the cap collects usable same-prefix members. New test test_progressive_inconsistency_uses_ols_descendants_fallback uses an OLS-shaped stub whose native descendants() is empty but whose fallback populates — the guard still flags it.

High #2 — EmptyReachableClosureError fired per-clause. Correct, and the minus-of-leaf case was a real false positive. Moved the raise out of _expand_reachable_from and up into expand_enum, so it fires only when the whole enum expands to nothing (matching the docs). New tests: reachable_from + permissible_values and minus:-rooted-at-leaf both expand non-empty and no longer abort.

Medium:

• Bump astral-sh/setup-uv from 5 to 7 #3 / feat: major documentation restructure and validation enhancements #6 _reverse_reaches now consults the OLS fallback on native error too (not just empty), and the fallback is capped by _REVERSE_FALLBACK_LIMIT so a near-root reverse closure under traverse_up is never fully paged — a truncated result stays UNANSWERABLE rather than falsely WITHOUT.
• Bump actions/github-script from 6 to 8 #4 the resolved_sources label lookup now runs only on the empty-expansion path, so the healthy path pays nothing.
• Bump actions/checkout from 4 to 5 #5 fixed as part of High Bump actions/setup-node from 3 to 6 #1 (filter inside the sampling loop).

Tests added for all three of your listed gaps: non-empty forward sample from an empty-native-descendants() adapter (#1 shape), CLI exit-3 via EmptyReachableClosureError (greedy), and reachable_from composed with minus: / permissible_values.

220 unit tests pass; mypy + ruff clean; OLS integration test passes against the fixed API.

---

Generated by Claude Code

[claude]

Claude finished @cmungall's task in 5m 1s —— View job

---

Re-review after c745f6a ("Address PR #69 re-review: OLS fallback in probe + per-enum empty check")

• Read the updated diff and surrounding code
• Check which prior review points were addressed
• Analyze correctness / performance / test coverage
• Post review

Both High findings from the last round are genuinely fixed, and fixed in the right place:

• Guard was a no-op on live OLS. _sample_closure now consults a bounded _ols_descendants (base.py:919-931, cap 8 × 8), and _ols_descendants grew a real limit that breaks out of the lazy paged crawl (base.py:1313-1319). _OlsLikeInconsistentAdapter pins the shape offline — native descendants() empty, REST fallback populated, ancestors omitting the root by CURIE — which is exactly the dismech#7012 signature.
• Per-clause → per-enum empty check. The raise moved out of _expand_reachable_from (now a clear NOTE at base.py:1263-1268) into expand_enum (base.py:1141-1153), gated on the merged value set. The minus-at-a-leaf and reachable_from + permissible_values false positives are gone and both have tests. Bonus: the get_ontology_label resolution lookup now runs only on the empty path, closing prior medium Bump actions/github-script from 6 to 8 #4. And because _closed_enum_caches.add sits after the raise, the in-memory empty set is never trusted as complete — good.

_REVERSE_FALLBACK_LIMIT also addresses the traverse_up crawl (prior medium #3), and the reverse-direction exception path now reaches the fallback (prior medium #6).

I could not run pytest / mypy / ruff in this job (bash approval), so everything below is from reading the code.

---

Medium — truncation off-by-one can still produce a false _REVERSE_WITHOUT

base.py:991:

if 0 < len(fallback) < self._REVERSE_FALLBACK_LIMIT:
    saw_any = True   # "exhausted the set without the target"

The truncation test is len(result) < limit, but _ols_descendants mutates the set after the cap: it breaks at len(values) >= limit and then does values.discard(source_node) (base.py:1321). So a crawl truncated at exactly 1024 that happened to include start_curie among the records returns 1023 → reads as "exhausted, demonstrably non-empty, target absent" → _REVERSE_WITHOUT → contributes to a hard abort on a set that was actually truncated. The discard is defensive (OLS's descendants endpoint shouldn't list the term itself), so probability is low — but this is precisely the truncated-evidence case the cap exists to avoid, and the failure mode is an exit-code-3 abort on a healthy ontology.

Cleanest fix: don't infer truncation from a length comparison. Fetch limit + 1 internally, or have _ols_descendants return/record whether it stopped early, and branch on that flag. Fix this →

---

Medium — forward sample and reverse probe now disagree about errored traversals

_reverse_reaches was deliberately changed to fall through to the OLS fallback when the native call raises (base.py:965-975, "mirroring the expansion path"). _sample_closure still return members inside its except (base.py:917), so an exception skips the fallback entirely.

Net effect on an OLS adapter whose descendants() raises rather than returning empty: the reverse direction recovers via REST, the forward sample does not → empty sample → guard silently disabled. That's the same silent no-op the commit set out to fix, one branch over. The docstring at base.py:978 even says "answers descendants() with nothing (or raises)" — the forward side doesn't honor that. Falling through to the fallback there (keeping the logger.warning) makes the two paths symmetric. Fix this →

---

Low

3. Fallback fires on a partial native sample, not only an empty one. base.py:919 triggers whenever len(members) < 8. A healthy OLS adapter whose descendants() legitimately returns 3 same-prefix members still pays a paged REST call per probed source node — and those 3 already suffice for consensus (any one that round-trips returns None immediately). if not members and method_name == "descendants" gets the no-op fix without the extra request.

4. Comment promises a warning, code logs debug. base.py:841-852 reads "…if the source is known to have children yet sampling found none, the guard is silently disabled — surface that" — then calls logger.debug. Either add the has-children check and warn, or drop the second half of the comment so it doesn't describe behavior that isn't there.

5. include:-nested reachable_from is outside the empty-closure guard. The check keys on enum_def.reachable_from (base.py:1141), so an enum whose only reachable_from lives in an include: branch (_expand_enum_expression, base.py:1170+) can still materialize and cache an empty-but-complete closure. Fail-safe and much narrower than the old over-firing behavior, but worth either extending or noting in the docs.

6. Reverse fallback has no target short-circuit. base.py:983-990 collects the full bounded set before testing target in fallback. Passing the target down so _ols_descendants stops the moment it appears would make the healthy traverse_up path cheap rather than "up to 1024 records × 8 members". Related: for any near-root term with >1024 descendants the verdict is permanently UNANSWERABLE — correct-by-design (fail-safe), but a logger.debug/warning noting "cap reached, consistency not determined" would make an effectively-off guard visible.

7. Sample isn't deduped. _accept (base.py:876-883) appends without a seen set; an adapter generator that yields a term twice burns a slot and re-probes the same member.

8. Docs wording now trails the code. docs/enumerations.md:203-208 says the check applies to "the whole query", but the implementation is stricter-scoped than that — it's the whole enum, including permissible_values, concepts, include and inherits contributions. Worth saying so explicitly (it's the more reassuring statement), along with the include:-nested caveat from #5.

---

Test coverage

The new tests land exactly on the prior findings: _OlsLikeInconsistentAdapter proves the guard fires through the REST fallback, test_greedy_minus_reachable_from_leaf_not_flagged and test_greedy_empty_reachable_from_plus_permissible_values_not_flagged lock the per-enum semantics, and the CLI test pins exit 3 for the empty-closure arm (both arms of the exit path are now covered).

Remaining gaps map onto the findings above:

• No boundary test for _reverse_reaches truncation (finding 1) — a stub client yielding exactly _REVERSE_FALLBACK_LIMIT records, one of them the start term, would pin it.
• No test for a native descendants() that raises on the forward sample (finding 2).
• The live-OLS integration test still exercises only _reverse_reaches; an assertion that _sample_closure returns a non-empty forward sample from the real bounded fallback would prove the new code path works against the actual API, not just the stub.

Nothing security-relevant; no new network surface, and the bounded crawls strictly reduce request volume versus the previous revision.
· branch claude/ltv-issue-7012-ed5bc9

[cmungall]

Round 3 — all findings addressed in 0976d2b.

Medium #1 (truncation off-by-one → false _REVERSE_WITHOUT). Real bug, fixed at the source: _ols_descendants now skips the source node during collection instead of discarding it afterwards, so a returned set of exactly the cap reliably means "truncated". _reverse_reaches treats a capped result as UNANSWERABLE (with a debug log noting the cap), and only an under-cap result as WITHOUT. Also added a stop_at short-circuit so a membership check succeeds even when the target is past the cap. New test_reverse_reaches_truncation_boundary pins truncated→UNANSWERABLE, exhausted→WITHOUT, and stop_at→FOUND (including the case where the REST page contains the start term).

Medium #2 (forward/reverse disagree on a raising native traversal). Fixed: _sample_closure no longer returns early from its except — it falls through to the bounded fallback, symmetric with _reverse_reaches. New test_progressive_inconsistency_when_native_descendants_raises uses an adapter whose native descendants() raises and still flags the inconsistency via the REST fallback.

Low:

• Bump astral-sh/setup-uv from 5 to 7 #3 fallback now fires only when the native traversal produced no usable members (a healthy adapter returning a few descendants pays no extra REST call).
• Bump actions/github-script from 6 to 8 #4 corrected the "surface that" comment — a childless leaf legitimately reaches nothing, so debug (not warn) is right.
• Bump actions/checkout from 4 to 5 #5 / feat: enable label validation by default #8 documented that the empty-expansion check keys on the top-level reachable_from (an include:-nested one is intentionally unguarded, fail-safe), and the docs now say "whole enum" (incl. permissible_values / concepts / include / inherits), not "whole query".
• feat: major documentation restructure and validation enhancements #6 the reverse fallback now short-circuits on the target (stop_at) and logs when the cap is hit without finding it.
• feat: add configurable cache strategies for dynamic enums #7 the forward sample is deduped so a generator yielding a term twice doesn't burn a slot.

On the integration test suggestion (assert _sample_closure returns a non-empty forward sample from the real bounded fallback): I left it out deliberately — on the now-fixed OLS API, native descendants(MONDO:0000001) returns members so the fallback path isn't exercised there, and forcing it would mean crawling the ~31k-descendant root over REST, which already tripped a 5s read-timeout earlier. The offline _OlsPagedAdapter covers that path deterministically instead.

222 unit tests pass; mypy + ruff clean; OLS integration test passes against the fixed API.

---

Generated by Claude Code

[claude]

Claude finished @cmungall's task in 2m 59s —— View job

---

Re-review after 0976d2b ("Address PR #69 re-review round 3: truncation, symmetry, and polish")

• Read the updated diff and surrounding code
• Check which prior review points were addressed
• Analyze correctness / performance / test coverage
• Post review

All eight round-3 findings are addressed, and the two that mattered are fixed at the root rather than patched:

• Truncation off-by-one. _ols_descendants now skips the source node during collection instead of discarding it afterwards (base.py:1344-1345), so len(result) == limit is a real truncation signal and < limit a real exhaustion signal. The stop_at short-circuit (base.py:1347-1348) additionally makes membership succeed past the cap, which also closes the old "reverse fallback has no target short-circuit" perf point. test_reverse_reaches_truncation_boundary pins all three arms (truncated → UNANSWERABLE, exhausted → WITHOUT, found-past-cap → FOUND) with the start term deliberately present in the page — exactly the case that used to come back one short.
• Forward/reverse symmetry. _sample_closure no longer returns from its except; a raising native descendants() now falls through to the bounded REST fallback (base.py:910-928), and test_progressive_inconsistency_when_native_descendants_raises locks it. The refactor of _OlsLikeInconsistentAdapter into a parameterized _OlsPagedAdapter is a good call — one stub, three shapes.

Also landed: fallback only on an empty native sample (if not members, base.py:928) so a healthy adapter returning a few descendants pays no extra REST call; sample dedup via seen (base.py:891-899); the cap-reached logger.debug (base.py:1008-1015); the comment at base.py:842-845 no longer promises a warning it doesn't emit; and the docs now say "whole enum … including permissible_values / concepts / include: / inherits:" with the include:-nested caveat stated in both the docs and a code NOTE (base.py:1160-1163).

I could not run pytest / mypy / ruff in this job (bash approval), so everything below is from reading the code.

---

Medium — the native reverse path still reports WITHOUT on truncated evidence

base.py:965-987:

saw_any = False
if callable(method):
    try:
        for term in method([start_curie], predicates=predicates):
            ...
            saw_any = True          # <-- set as terms stream in
            if term == target: return self._REVERSE_FOUND
    except Exception as e:
        raise_if_service_unavailable(start_curie, e)
        logger.warning(...)         # <-- falls through with saw_any already True

if not saw_any and method_name == "descendants":   # <-- fallback skipped
    ...
return self._REVERSE_WITHOUT if saw_any else self._REVERSE_UNANSWERABLE

If the native generator yields a few terms and then raises (lazy paging that fails on page 2, an adapter that chokes on one malformed record, a KeyError mid-stream), saw_any is already True, so the closure is reported as demonstrably non-empty and without the target — a _REVERSE_WITHOUT computed from a truncated closure that may well have contained the source further along. Non-connectivity exceptions survive raise_if_service_unavailable (it only re-raises connectivity errors and 5xx/408/429, oak_utils.py:212-216), so a RuntimeError/ValueError mid-iteration reaches exactly this path. Two knock-ons: the REST fallback is skipped precisely when the native answer is least trustworthy, and enough such members make answerable=True in _compute_inconsistency_witness (base.py:860-867) → exit-code-3 abort on a healthy ontology.

This is the same "truncated evidence must not read as exhausted" invariant the commit just established for the REST path — it needs to hold on the native path too. Simplest fix: track errored = True in the except and either force the fallback (if (not saw_any or errored) and method_name == "descendants") or return _REVERSE_UNANSWERABLE when errored and the target was never seen. Fix this →

---

Medium — the probe's sample is nondeterministic, so the verdict can be too

_ols_descendants returns a set; _sample_closure iterates it directly (base.py:938-940) to pick _INCONSISTENCY_SAMPLE_SIZE (8) members out of up to 8 × 8 = 64 collected. Python's string hash is randomized per process, so which 8 members get probed differs run to run.

For a uniformly broken adapter (the dismech#7012 shape — every descendant misses the root) that's harmless. But on a partially broken or mixed graph — some members round-trip, some don't — the consensus rule short-circuits on the first FOUND (base.py:858-859), so one run samples a healthy member and returns None while the next samples 8 broken ones and raises InconsistentReachabilityError. A hard abort that flips between runs on identical inputs is a bad failure mode to debug, and it also makes any future live-OLS assertion on this path flaky. for term in sorted(fallback): costs nothing at n≤64 and makes the sample reproducible. Fix this →

---

Low

3. The new "trying fallback" warning lies on the traverse_up path. base.py:917-923 logs "could not sample %s([%s]) natively; trying fallback" for any errored traversal, but the fallback at base.py:928 is gated on method_name == "descendants". Under traverse_up=True the forward method is ancestors, so nothing is tried and the guard is silently disabled for that source node — with a log line saying the opposite. Either mention the method in the "no fallback available" case or keep the old "consistency guard disabled for this source node" wording on the non-descendants branch.

4. Docstring overstates the truncation signal. base.py:1305-1309: "a returned set of exactly limit … reliably signals a truncated crawl". A crawl that exhausts at exactly limit distinct descendants returns limit too and reads as truncated. The consequence is _REVERSE_UNANSWERABLE, i.e. fail-safe and correct behavior — but "reliably" should be "conservatively: == limit means possibly truncated, and callers must not treat it as exhausted."

5. Prefix filtering still happens after the fallback's cap. Round 3 moved filtering inside the sampling loop for the native path, but _ols_descendants has no prefix knowledge, so its bounded crawl still collects 64 members before _accept drops foreign-prefix ones (base.py:931-940). The 8× headroom makes this fine in practice; worth a one-line comment saying that's what the * 8 is for (the current comment says "survive prefix filtering", which is right — just easy to lose in a later refactor that tightens the multiplier).

---

Test coverage

The three new/reworked tests land squarely on the prior findings, and _OlsPagedAdapter is a nice consolidation. Remaining gaps:

• No test for a native traversal that yields then raises (finding 1) — a generator stub yielding two terms before a RuntimeError would pin _REVERSE_UNANSWERABLE and prevent the truncated-evidence abort.
• No exhausted-at-exactly-the-cap case. test_reverse_reaches_truncation_boundary covers truncated-at-cap and exhausted-below-cap; the third boundary (a page with exactly _REVERSE_FALLBACK_LIMIT genuine members and no more) is the one where == is ambiguous, and asserting UNANSWERABLE there documents the deliberate fail-safe choice.
• The live-OLS integration test still only exercises _reverse_reaches (tests/test_oak_providers_integration.py:132-138). The forward bounded fallback — the code path that actually makes the guard fire on ols: — is pinned only by an offline stub. Now that _sample_closure caps at 64 records it's cheap to assert live: assert plugin._sample_closure(adapter, "descendants", "MONDO:0000001", ["rdfs:subClassOf"], "MONDO") is non-empty.

Nothing security-relevant; stop_at and the source-skip strictly reduce request volume versus the previous revision, and the double-quoted IRI encoding is unchanged.
· branch claude/ltv-issue-7012-ed5bc9

[cmungall]

Round 4 — all findings addressed in 78d75dc.

Medium #1 (native reverse path reports WITHOUT from truncated evidence). Correct, and it's the same invariant I'd just established for the REST path — it needed to hold natively too. _reverse_reaches now tracks an errored flag: a native traversal that yields then raises no longer lets saw_any drive a WITHOUT verdict. An errored partial read forces the bounded fallback (descendants direction) and otherwise resolves to _REVERSE_UNANSWERABLE. New test_reverse_reaches_native_error_midstream_is_unanswerable (reverse = ancestors, no fallback → must be UNANSWERABLE) pins it.

Medium #2 (nondeterministic sample). Correct — _ols_descendants returns a set and iterating it directly meant process-randomized hashing chose the sampled members, so a mixed graph's consensus verdict could flip run-to-run (a hard abort flipping on identical input is a nasty failure mode). Now iterates sorted(fallback).

Low:

• Bump astral-sh/setup-uv from 5 to 7 #3 reworded the native-sampling warning so it no longer implies a fallback exists for non-descendants directions (traverse_up forward = ancestors).
• Bump actions/github-script from 6 to 8 #4 softened the _ols_descendants truncation docstring — == limit means possibly truncated, never read as exhausted.
• Bump actions/checkout from 4 to 5 #5 documented the * 8 fallback headroom (it's what lets the prefix filter drop foreign-prefix terms and still leave enough usable members).

Tests for all three coverage gaps you listed: native-yields-then-raises → UNANSWERABLE; exhausted-at-exactly-the-cap → UNANSWERABLE (the ambiguous boundary, documenting the fail-safe choice); and the live-OLS integration test now asserts the forward _sample_closure returns real same-prefix members — the path that actually fires the guard on ols:, previously pinned only by the offline stub. Verified passing against the live API.

223 unit tests pass; mypy + ruff clean.

---

Generated by Claude Code

[claude]

Claude finished @cmungall's task in 4m 6s —— View job

---

Re-review after 78d75dc ("Address PR #69 re-review round 4: truncated native evidence + determinism")

• Read the updated diff and surrounding code
• Check which prior review points were addressed
• Analyze correctness / performance / test coverage
• Post review

Both round-4 findings are fixed, and I traced the restructured _reverse_reaches branch-by-branch against the round-3 version — it is behavior-preserving on every previously-reachable arm:

• Truncated native evidence. The errored flag (base.py:972, 990) now means a native traversal that yields terms and then raises can no longer produce _REVERSE_WITHOUT. The gate widened to (not saw_any or errored) (base.py:1003), so a partially-read descendants closure also reaches the bounded REST fallback instead of being trusted. The three fallback arms became explicit early returns (WITHOUT / cap-hit UNANSWERABLE / empty→fall-through) — same verdicts as before, but now readable, and the final if saw_any and not errored is the only place a native-only WITHOUT can come from. test_reverse_reaches_native_error_midstream_is_unanswerable pins the ancestors (no-fallback) arm.
• Nondeterministic sample. sorted(fallback) at base.py:944 fixes the process-randomized set iteration on the REST path.
• Also landed: the exhausted-at-exactly-the-cap boundary case in test_reverse_reaches_truncation_boundary (adapter4 → UNANSWERABLE), the _ols_descendants docstring downgraded from "reliably signals truncated" to the honest "may have been truncated … must NOT be read as exhaustive" (base.py:1326-1329), and the live-OLS assertion on the forward sample (tests/test_oak_providers_integration.py:139-144) — which is the one that proves the guard isn't a no-op against the real API.

I could not run pytest / mypy / ruff in this job (bash approval was declined for uv run and python -c), so everything below is from reading the code.

---

Medium — EmptyReachableClosureError now misdiagnoses when minus: empties a populated closure

Moving the check to the merged per-enum set (correct, and it killed the real false positives) means values at base.py:1182 is evaluated after minus: is applied at base.py:1141-1143. So this config:

DiseaseEnum:
  reachable_from:
    source_nodes: [MONDO:0000001]      # → 20k terms
  minus:
    - concepts: [...]                   # → happens to cover all of them

raises with:

reachable_from source node 'MONDO:0000001' resolves to a valid term but the query expands to an empty set (its descendant closure is empty under the configured adapter), so every MONDO term would be silently rejected. This usually means the source node or adapter is misconfigured. Verify the source node, or configure a local adapter such as 'sqlite:obo:mondo'.

Every clause after the first is false here: the closure was large, the adapter is fine, and sqlite:obo:mondo will produce exactly the same empty enum. The user is sent to debug an adapter when the cause is their own set arithmetic. Failing loud is still defensible (the enum genuinely matches nothing and would cache empty-but-complete), so I'd keep the raise and fix the attribution: capture whether the top-level reachable_from clause itself came back empty — rf_values = self._expand_reachable_from(...) at base.py:1125 already has it in hand — and branch the message between "the source's closure is empty (check the source node / adapter)" and "the closure was non-empty but the enum's minus:/set operations removed everything". docs/enumerations.md:223-227 has the same gap: the "fires only when the entire query expands to nothing" paragraph lists the leaf-source case but not the subtractive one. Fix this →

---

Low

2. The determinism fix covers only half the sampling path. _sample_closure now sorts the REST fallback (base.py:944) but the native path (base.py:905-907) still probes members in whatever order the adapter's generator yields, and several oaklib implementations build a closure via set/graph traversal before yielding — so on a native adapter the "which 8 of N" question is still answered nondeterministically, and the short-circuit-on-FOUND consensus rule (base.py:858-859) can still flip the verdict between runs on a mixed graph. The two paths also now use different sampling policies: the fallback over-collects 64 and sorts down to 8, the native path takes the first 8 in stream order. Making them symmetric — collect up to _INCONSISTENCY_SAMPLE_SIZE * 8 lazily, then sorted(...)[:8] — costs nothing at n ≤ 64 and keeps the early bound. (Worth noting this trades against the current "stop the generator at 8" laziness; if you'd rather keep that, a one-line comment saying the native sample order is adapter-defined would at least stop the next reader from assuming the sorted() covers both.) Fix this →

3. _ols_descendants(reflexive=True, limit=N) can return N+1. The cap breaks at len(values) >= limit (base.py:1369) and then values.add(source_node) runs unconditionally (base.py:1371-1372). No current caller combines reflexive with limit, so this is latent — but it silently voids the docstring contract the round-3/4 work just established (== limit ⇒ possibly truncated), and the greedy expansion path is the one that passes reflexive=include_self. Either assert the two are never combined or subtract one from the effective cap when reflexive.

4. The reworded probe warning still applies its own caveat unconditionally. base.py:919-921 emits "(a descendants fallback is attempted; other directions are skipped)" for every errored sample, so the reader has to cross-reference method_name in the same message to know which half applies. Since method_name is right there, branching it — "; trying the descendants fallback" vs "; no fallback for this direction, so the consistency guard is disabled for this source node" — makes a silently-disabled guard actually greppable, which was the point of promoting it to warning.

5. Test mutates a class constant through the instance. plugin._REVERSE_FALLBACK_LIMIT = 3 (tests/test_reachable_from_progressive.py:433) works only because the production code reads it as self._REVERSE_FALLBACK_LIMIT. A later refactor to BaseOntologyPlugin._REVERSE_FALLBACK_LIMIT or type(self).... would make the boundary test silently stop testing the boundary (all four asserts would still pass, against a 1024 cap). monkeypatch.setattr(DynamicEnumPlugin, "_REVERSE_FALLBACK_LIMIT", 3) is immune.

---

Test coverage

The two new tests land on the two round-4 findings, and adapter4 closing the exactly-at-cap boundary is a good addition. One gap remains, and it's specifically in the code this commit added:

• The saw_any=True and errored=True descendants arm is untested. _OlsPagedAdapter(native="raise") raises on the first call, so saw_any is False — the new or errored half of the gate at base.py:1003 is never exercised, and neither is the "partial native view, recovered by the fallback" path it exists for. _YieldsThenRaisesAdapter has the right shape but its yields-then-raises method is ancestors, which has no fallback. A stub whose descendants() yields two terms then raises, with a REST page containing the target, would pin _REVERSE_FOUND (and a variant without the target, _REVERSE_WITHOUT from the fallback rather than from the truncated native view) — i.e. lock in that the fallback overrides truncated native evidence rather than merely supplementing it. Fix this →

Nothing security-relevant. stop_at, the source-skip, and the if not members fallback gate all strictly reduce request volume versus earlier revisions; the double-quoted IRI encoding is unchanged.

Overall this is in good shape — the remaining items are diagnostics, symmetry and one latent contract violation, none of which can produce the false abort the earlier rounds were about.
· branch claude/ltv-issue-7012-ed5bc9

[cmungall]

Round 5 — all addressed in 443d1c2.

Medium (minus: misdiagnosis). Good catch — failing loud is still right, but the message was blaming the adapter for the user's set arithmetic. expand_enum now captures whether the top-level reachable_from itself expanded to nothing (rf_values) and passes source_closure_empty to EmptyReachableClosureError, which branches between "bad source node / adapter" and "the enum's minus:/set operations removed every term — review the minus:/include: clauses". Docs updated with the subtractive case. New test_greedy_reachable_from_cancelled_by_minus_reports_set_arithmetic asserts source_closure_empty is False and the set-arithmetic wording.

Low #2 (sampling determinism only covered the REST path). Fixed by making both paths symmetric: _sample_closure now lazily collects a bounded pool (up to sample_size * 8) from the native path as well, then returns sorted(pool)[:sample_size]. The consensus verdict no longer depends on process-randomized adapter/set iteration order on a mixed graph, and the two paths share one policy.

Low #3 (_ols_descendants(reflexive=True, limit=N) → N+1). Fixed: it now reserves a slot for the reflexive source (skips the source during collection, caps genuine descendants at limit-1 when reflexive), so len == limit stays a reliable truncation signal for every caller.

Low #4 (warning caveat). Branched on method_name: "trying the descendants fallback" vs "no fallback for this direction, so the consistency guard is disabled for this source node" — a disabled guard is now greppable.

Low #5 (test mutated a class constant via the instance). Switched to monkeypatch.setattr(DynamicEnumPlugin, "_REVERSE_FALLBACK_LIMIT", 3).

Test gap (saw_any=True and errored=True descendants arm). Added test_reverse_reaches_fallback_overrides_truncated_native: native descendants() yields two terms then raises, and the bounded REST fallback is authoritative — FOUND when its page contains the target, WITHOUT when it exhausts without it. Proves the fallback overrides truncated native evidence rather than supplementing it.

225 unit tests pass; mypy + ruff clean; live-OLS integration test passes.

Thanks for the branch-by-branch trace on _reverse_reaches — matches my read that the restructure is behavior-preserving on the previously-reachable arms.

---

Generated by Claude Code

[claude]

Claude finished @cmungall's task in 3m 57s —— View job

---

Re-review after 443d1c2 ("Address PR #69 re-review round 5: error attribution, sampling symmetry, polish")

• Read the updated diff and surrounding code
• Check which round-4 review points were addressed
• Analyze correctness / performance / test coverage
• Post review

Both round-4 items are addressed, and the attribution fix landed in the right place:

• Error attribution. expand_enum now keeps the top-level reachable_from contribution in rf_values (base.py:1134-1140) and passes source_closure_empty=not rf_values into the error (base.py:1210), which branches the message between "bad source node / adapter" and "your minus:/set operations removed every term" (oak_utils.py:85-103). test_greedy_reachable_from_cancelled_by_minus_reports_set_arithmetic pins both the flag and the wording. I also re-checked that the in-memory self._enum_cache[enum_name] = values write that precedes the raise (base.py:1174) can't leak the empty set — the dynamic fast path requires enum_name in self._closed_enum_caches, and that add is still after the raise. Good.
• Sampling symmetry / reflexive+limit. The pool-then-sorted(pool)[:8] restructure makes the two paths share one policy, and reserving a slot for the reflexive source (base.py:1375-1381, plus the unconditional source skip during collection) keeps len == limit meaning "possibly truncated" for every caller.
• The saw_any=True and errored=True descendants arm now has a test (test_reverse_reaches_fallback_overrides_truncated_native), and the boundary test monkeypatches the class attribute.

I could not run pytest / mypy / ruff in this job (bash approval was declined for uv run), so everything below is from reading the code.

---

Medium — include_self: true silently disables the empty-closure guard

_expand_reachable_from passes reflexive=include_self (base.py:1287-1298), so with

DiseaseEnum:
  reachable_from:
    source_nodes: [MONDO:0000001]
    include_self: true

a broken adapter that reaches nothing still yields rf_values == {"MONDO:0000001"} → values is non-empty → if not values and enum_def.reachable_from (base.py:1187) never fires. The enum becomes a one-term enum that rejects every real MONDO term as "not in enum", and — because the raise is skipped — _closed_enum_caches.add + _save_enum_cache run, persisting it as an empty-but-complete closure. That is precisely the two silent failures EmptyReachableClosureError's docstring says it exists to prevent, and docs/enumerations.md's "fires when the whole enum expands to nothing" reads as covering it.

include_self defaults to False (base.py:277-279), so this only bites schemas that set it explicitly — but those are exactly the schemas where the source term is expected to be one of many. Treating "the enum's only values are the reflexive source node(s)" as empty for guard purposes closes it without affecting any legitimate config (an enum whose sole intended member is the source node itself would be written as permissible_values/concepts, not reachable_from). Fix this →

---

Medium — the new set-arithmetic message is wrapped in three layers that still say "adapter"

The inner message is now accurate, but nothing around it changed:

layer	text	true for source_closure_empty=False?
EmptyReachableClosureError docstring (oak_utils.py:64-73)	"Raised when a reachable_from source node resolves but reaches nothing"	no — the closure was large
UnreliableReachabilityError docstring (oak_utils.py:53-60)	"membership could not be reliably computed because the configured adapter returned a broken graph"	no — the adapter was fine
CLI banner (cli.py:77-83)	"dynamic-enum reachability is unreliable … Reachability could not be computed reliably, so terms were not checked"	no — reachability computed fine; the enum is empty by design

So a user who hits the minus:-cancels-everything case reads a correct middle sentence sandwiched between two incorrect ones, and gets exit code EXIT_ONTOLOGY_MISCONFIGURED. Exit 3 is still defensible (it is a configuration problem, and it will not fix itself on retry), but the framing should follow the branch — e.g. a sibling exception under UnreliableReachabilityError (EmptyEnumAfterSetOperationsError) so _fail_unreliable_reachability can pick a banner, or at minimum soften the wrapper text to "the enum matched nothing, so terms could not be meaningfully checked" and adjust the two docstrings. Fix this →

---

Low

3. "Deterministic on both paths" is only true when the closure fits the pool. _sample_closure's docstring (base.py:884-891) claims sorting makes the sample deterministic, but the pool is the first pool_cap (64) same-prefix members in adapter order (base.py:905-908). For a closure with more than 64 usable members — a MONDO root, i.e. the motivating case — which 64 land in the pool is still adapter/set-order dependent, so sorted(pool)[:8] can still differ between runs and the consensus verdict can still flip on a mixed graph. The REST path is fully deterministic (the fallback limit equals pool_cap, so every fetched member is considered). Full determinism would require a full walk, which the cap deliberately avoids — so the honest fix is the same one applied to the truncation docstring last round: say the sample is deterministic given the pool, and that the pool is adapter-ordered for closures above the cap.

4. Sorting biases the sample toward near-root terms, worst case under traverse_up. sorted(pool)[:8] picks the numerically-lowest CURIEs, and in most OBO ontologies low IDs are the oldest, most root-like terms. Under traverse_up the sampled members are the source's ancestors and the reverse direction is descendants, so the sort systematically selects the members with the largest reverse closures — up to 8 × _REVERSE_FALLBACK_LIMIT (1024) records before the first verdict, where an arbitrary sample would usually have picked cheaper mid-tree terms. Evenly-spaced indices over the sorted pool (sorted(pool)[::max(1, len(pool)//8)][:8]) stays deterministic and spreads the sample across the ID range.

5. The native path now walks 8× further than it needs to. Previously the generator stopped at 8 members; it now runs to pool_cap = 64 (base.py:894, _collect returns True only at the cap) even though only 8 are used, and the consensus rule short-circuits on the first member that round-trips. Cheap on sqlite:, but it's a per-probed-source cost increase on any paging adapter, taken purely to improve sampling spread. Worth a comment saying that's the trade being made.

6. The errored-sample warning promises a fallback that may not run. base.py:912-918 logs "trying the descendants fallback" from inside the except, but the fallback is gated on if not pool (base.py:929). A native traversal that yields a few terms and then raises has a non-empty pool → no fallback → the log says otherwise. Moving the log below the fallback decision (or wording it "will try … if nothing was sampled") keeps the greppability goal without the false statement. (The truncated pool itself is fine as evidence — the members that were yielded are genuine, so unlike _reverse_reaches truncation here cannot manufacture a false witness.)

7. _ols_descendants(reflexive=True, limit=1) can still return 2. The cap test runs after values.add (base.py:1387-1388), so effective_limit=0 still admits one descendant, then values.add(source_node) makes two. Latent only — every caller passes 64 or 1024 — but it's the same contract (len == limit) the reservation was added to protect; if effective_limit is not None and len(values) >= effective_limit: break placed before the add, or an explicit if effective_limit == 0: break, closes it.

---

Test coverage

The three new/updated tests land squarely on the round-4 findings, and _OlsPagedAdapter(native="yield_then_raise") is a clean way to reach the saw_any and errored arm — with both the FOUND and WITHOUT outcomes asserted, which is what proves the fallback overrides rather than supplements the truncated native view.

Gaps that map onto the above:

• No test for include_self: true over an empty/broken closure (finding 2) — an offline reachable_from on TEST:0000004 (childless leaf) with include_self: true would currently expand to {"TEST:0000004"}, cache complete, and not raise. That test would fail today and pass after the fix.
• No test for _sample_closure's new pool/sort behavior. An adapter yielding, say, 20 same-prefix members in descending order and asserting the returned sample is the 8 lowest sorted would pin the round-5 restructure; today nothing would catch a regression back to "first 8 in stream order".
• No CLI-level test for the set-arithmetic arm. tests/test_cli.py:214 / :265 cover exit 3 for the inconsistency and empty-closure arms; the new branch shares the exit path but has different user-facing text, and per finding 3 that text is currently self-contradictory — a CLI test is what would have surfaced it.

Nothing security-relevant; the round-5 changes don't add network surface (the effective_limit reservation strictly reduces records fetched; the native pool walk is local iteration on an already-open traversal).

Overall this continues to converge — nothing left in this round can produce the false hard-abort the earlier rounds were about; finding 2 is the one that leaves the guard's stated contract unmet, and the rest are diagnostics and doc accuracy.
· branch claude/ltv-issue-7012-ed5bc9

[cmungall]

Round 6 — all addressed in 52f036c.

Medium #1 (include_self: true disabled the empty-closure guard). Real contract gap, good catch. The guard now subtracts the top-level reachable_from source nodes before the emptiness test — an enum whose only members are its own reflexive sources is treated as empty and flagged (source_closure_empty=True). New test_greedy_include_self_over_childless_source_still_flagged fails on the old code, passes now, and confirms no complete marker is written.

Medium #2 (set-arithmetic message wrapped in adapter-blaming layers). Fixed all three layers: softened the UnreliableReachabilityError and EmptyReachableClosureError docstrings to cover both causes, and branched the CLI banner on source_closure_empty — the minus:-cancels case now prints "a dynamic enum matched nothing … set operations removed every term … a schema problem, not invalid data or a broken adapter" instead of the reachability banner. New CLI test asserts the set-arithmetic banner and that the adapter banner is absent.

Low:

• Bump actions/github-script from 6 to 8 #4 _sample_closure returns an even-stride sample over the sorted pool, not the lowest CURIEs, so it's not biased toward the most root-like (most expensive under traverse_up) terms.
• Bump astral-sh/setup-uv from 5 to 7 #3 docstring now scopes determinism to "given the pool" (adapter-ordered above the cap), matching the truncation-docstring honesty from last round.
• Bump actions/checkout from 4 to 5 #5 added the collect-to-64 trade-off comment.
• feat: major documentation restructure and validation enhancements #6 reworded the errored-native warning ("fallback tried only if nothing was sampled") so it doesn't promise a fallback a non-empty partial pool skips.
• feat: add configurable cache strategies for dynamic enums #7 _ols_descendants checks the cap before adding, so reflexive+limit==1 can't return 2.

Tests for all three gaps: the include_self case, _sample_closure's sort+even-stride determinism (adapter yields 20 in descending order → asserted sorted even-stride subset), and the CLI set-arithmetic banner.

228 unit tests pass; mypy + ruff clean; live-OLS integration test passes.

As you noted, this is well past the false-abort territory now — round 6 was one genuine contract gap (include_self) plus message/diagnostic accuracy. Happy to keep polishing, but from my side the guard is correct and the remaining surface is cosmetic.

---

Generated by Claude Code

[claude]

Claude finished @cmungall's task in 3m 34s —— View job

---

Re-review after 52f036c ("round 6: include_self guard, message attribution, sampling")

• Read the updated diff and surrounding code
• Check which round-5 review points were addressed
• Analyze correctness / performance / test coverage
• Post review

All three round-5 items landed, and the attribution work is now consistent end to end:

• include_self no longer masks the guard — expand_enum subtracts the reachable_from source nodes before the emptiness test (base.py:1223-1226), and test_greedy_include_self_over_childless_source_still_flagged asserts both the raise and that no .complete marker is written.
• Message attribution — the UnreliableReachabilityError/EmptyReachableClosureError docstrings now cover both causes (oak_utils.py:53-83), and _fail_unreliable_reachability branches the banner on source_closure_empty (cli.py:69-97). The new CLI test asserts the set-arithmetic banner and that the adapter banner is absent — that negative assertion is what makes it a real lock.
• Sampling / _ols_descendants — the docstring now scopes determinism to "given the pool" rather than claiming it outright, the errored-sample warning no longer promises a fallback a non-empty partial pool skips, and the cap moved before values.add so reflexive + limit can't return limit + 1.

I could not run pytest / mypy / ruff in this job (bash approval was declined for uv run), so everything below is from reading the code.

---

Medium — the source-node subtraction hard-aborts a valid multi-source enum

base.py:1223-1231 subtracts all reachable_from source nodes from the merged value set, unconditionally — not just the ones added reflexively. So any enum whose closure members happen to be the other source nodes now comes out "empty". With this repo's own fixture ontology (TEST:0000004 is_a TEST:0000002, and 0000004 is 0000002's only descendant):

MyEnum:
  reachable_from:
    source_nodes: [TEST:0000002, TEST:0000004]   # a parent and its only child

values = {"TEST:0000004"} → rf_source_nodes = {"TEST:0000002", "TEST:0000004"} → effective_values = ∅ → EmptyReachableClosureError with source_closure_empty=True, i.e. "its descendant closure is empty under the configured adapter … configure a local adapter such as sqlite:obo:test" — on a perfectly healthy local simpleobo adapter, for an enum that legitimately has one member. Listing a branch root together with a specific sub-branch is a normal hand-curated pattern; it exits 3 today.

The same shape bites without include_self at all (concepts: [X] alongside reachable_from: source_nodes: [X] where the closure is a legitimate leaf), because the subtraction can't tell which clause put a source node into values.

Set subtraction over the merged values isn't quite the right signal — the question the guard actually wants answered is "did any source's traversal yield anything at all?". _expand_reachable_from has that in hand; returning it (a bool alongside the set, or a small result object) and gating the include_self arm on it would flag the childless-source case while leaving the parent+child union alone:

if not values or (not any_traversal_yield and values <= rf_source_nodes):

Fix this →

---

Low

2. The error now names a nondeterministic source node. resolved = next((sn for sn in rf_source_nodes if …), None) (base.py:1228-1230) iterates a set, where the previous revision iterated enum_def.reachable_from.source_nodes (a list). With more than one resolvable source, which node the message blames — and the order of the get_ontology_label lookups that precede it — is process-randomized, which is the exact property rounds 4–5 spent effort establishing elsewhere (sorted(fallback), the _ols_descendants source-skip). next((sn for sn in (enum_def.reachable_from.source_nodes or []) if sn in rf_source_nodes and …)) restores it for free. Fix this →

3. The even stride degenerates to "lowest 8" for small pools. stride = max(1, len(ordered) // 8) then ordered[::stride][:8] (base.py:977-978): for a pool of 9–15 members the stride is 1, so the sample is exactly the 8 lowest CURIEs — the root-like bias the change set out to remove — and for a pool of, say, 20 the stride-2 slice is truncated at index 14, leaving the top quarter unsampled. Only pools that are exact multiples of the sample size span the full range. [ordered[round(i * (len(ordered) - 1) / (k - 1))] for i in range(k)] (dedup, k = min(8, len)) is deterministic and actually spans it. Worth noting the current docstring at base.py:891-895 states the spread property unconditionally.

4. Docs and code disagree about what's subtracted. docs/enumerations.md:220-224 describes the new behavior as "an enum whose only member would be its own reflexive source node", but the code subtracts the source nodes regardless of include_self and regardless of which clause contributed them (see finding 1). Either narrow the code to the reflexive contribution or widen the doc sentence.

5. stop_at is no longer consulted for the record that hits the cap. Moving the cap test above values.add (base.py:1420-1424) means the record at slot effective_limit + 1 is dropped before its obo_id == stop_at check, where the previous order added it and short-circuited. Fail-safe (→ _REVERSE_UNANSWERABLE), one record wide, and the reflexive-slot fix it enables is worth more — just worth a comment so a future reader doesn't "restore" the add-then-check order.

---

Test coverage

The three new tests land on the round-5 findings, and _ManyDescAdapter yielding in descending order is the right way to pin the sort (a regression to stream order would fail loudly). Gaps:

• No multi-source test where the closure members are themselves source nodes (finding 1) — source_nodes: ["TEST:0000002", "TEST:0000004"] would raise today and shouldn't.
• No positive control for the new subtraction: nothing asserts that include_self: true over a populated source (e.g. TEST:0000001) still expands and caches normally. Right now only the flagged case is pinned, so a subtraction that over-fires has no test standing against it.
• No test for the source node named in the message with several resolvable sources (finding 2) — assert exc.source_node == "..." over a two-source enum would pin determinism.
• test_sample_closure_is_sorted_and_evenly_spaced computes the expected slice with the same stride expression as the implementation, so it locks the shape but would pass unchanged if the stride formula were wrong (e.g. the degenerate small-pool case in finding 3). A literal expected list, plus a second call asserting equality across two invocations, would test the property rather than restate the code.

Nothing security-relevant; round 6 adds no network surface (the cap reordering strictly reduces records collected, and the pool walk is local iteration on an already-open traversal).

Overall: the guard's contract is now met for include_self, and the user-facing messaging finally matches the cause on every layer. Finding 1 is the one thing I'd want fixed before merge — it's the same false-hard-abort class the earlier rounds were about, just arriving from the new subtraction rather than from the probe.
· branch claude/ltv-issue-7012-ed5bc9

[cmungall]

Round 7 — fixed in e647800. You were right that finding 1 was a real false-abort I introduced in round 6, so this was worth catching before merge.

Medium (source-node subtraction false-aborts a valid union). Replaced the subtraction with the signal the guard actually wants: _expand_reachable_from_detailed now reports whether any source's traversal reached a term other than the source itself (a member that happens to be another source node still counts — the parent+child case). The guard fires only when:

• the merged value set is truly empty, or
• the traversal reached nothing real and the enum's only members are its own source nodes and no concepts/permissible_values/include/inherits clause could have contributed them.

So include_self over a childless source is still caught, but source_nodes: [parent, child] and concepts:[X] + reachable_from:[X-leaf] are not. (I gated on the "no other value clause" condition too — the bare values <= rf_source_nodes you sketched would still have false-flagged the concepts:[X] case you flagged in passing.)

Low:

• Bump actions/setup-python from 5 to 6 #2 the flagged source node is now chosen from the declared source_nodes list, not the set, so the message is deterministic.
• Bump astral-sh/setup-uv from 5 to 7 #3 _sample_closure uses evenly-spaced indices spanning the sorted pool (endpoints included) instead of the stride slice that degenerated to "lowest k" for small pools.
• Bump actions/github-script from 6 to 8 #4 docs corrected to describe the actual (reached-something) check, incl. the parent+child union carve-out.
• Bump actions/checkout from 4 to 5 #5 added the stop_at/cap-ordering comment in _ols_descendants.

Tests for every gap you listed: parent+child union not flagged; positive control that include_self:true over a populated source expands and caches normally; a two-source empty enum names a stable source node; and _sample_closure now asserts a literal evenly-spaced sample that spans the range and is equal across two calls (tests the property, not the formula).

231 unit tests pass; mypy + ruff clean; live-OLS integration test passes.

---

Generated by Claude Code

[claude]

Claude finished @cmungall's task in 2m 44s —— View job

---

Re-review after e647800 ("Fix round-6 regression: don't false-abort a valid multi-source union")

• Read the updated diff and surrounding code
• Check which round-6 review points were addressed
• Analyze correctness / performance / test coverage
• Post review

The round-6 regression is fixed properly, and fixed at the right level of the stack: instead of inferring emptiness by subtracting source nodes from the merged set, _expand_reachable_from_detailed now reports whether any source's traversal yielded a term other than the source itself (base.py:1378-1382), and expand_enum gates the include_self arm on that flag plus "no other clause could have contributed these values" (base.py:1238-1247). The parent+child union and concepts:[X] + leaf-reachable_from cases are no longer flagged, and test_greedy_multi_source_parent_and_child_union_not_flagged plus the include_self-over-a-populated-source positive control pin both directions of the guard — the positive control being the one that stands against future over-firing.

Also landed: the flagged source node is picked from the declared source_nodes list rather than a set (base.py:1250-1253), so both the message and the get_ontology_label lookup order are stable; _sample_closure replaced the stride slice with evenly-spaced indices spanning the pool (base.py:975-979) — I checked the arithmetic for n = 9, 10, 12, 20 at k=8 and it yields exactly 8 distinct indices with both endpoints included, and the test now asserts a literal expected list rather than restating the formula; and the stop_at/cap ordering in _ols_descendants has a NOTE explaining why it must not be "restored".

I could not run pytest / mypy / ruff in this job (bash approval was declined for uv run), so everything below is from reading the code.

---

Medium — include_self: true on a live OLS adapter now hard-aborts a healthy ontology

_expand_reachable_from_detailed reaches the bounded OLS REST fallback only when the native traversal is falsy:

result = self._call_graph_traversal(..., reflexive=include_self)   # base.py:1364
if not result:                                                     # base.py:1372
    result = self._ols_descendants(...)

But _call_graph_traversal adds the start CURIE unconditionally under reflexive (base.py:761-762). So on an OLS adapter whose descendants() returns nothing — the exact condition this PR built _ols_descendants and the whole _sample_closure fallback for — include_self: true makes result == {source_node}, which is truthy, and the fallback is skipped. Then:

• rf_reached is False (result - {source_node} is empty)
• values == {source_node} → values <= rf_source_nodes
• no other clause → EmptyReachableClosureError(source_closure_empty=True) → exit 3, "its descendant closure is empty under the configured adapter … configure a local adapter such as sqlite:obo:mondo"

...on an ontology where the identical enum without include_self expands fine through the fallback. The falsy-gate is pre-existing, but its consequence is new: before round 6 this silently produced a one-term enum, and now it's a hard abort blaming a healthy adapter — the same false-abort class rounds 5–7 have been closing.

The fix matches the signal the commit just introduced: gate the fallback on whether anything real was reached, not on truthiness — if not (result - {source_node}):. That also keeps reached_something honest for the OLS path. Fix this →

---

Low

2. matches is missing from has_other_value_clause. base.py:1238-1243 lists concepts / permissible_values / include / inherits, but expand_enum also merges matches (base.py:1167-1169). An enum with reachable_from (include_self: true, childless source) plus a matches: clause that happens to resolve to nothing falls into arm (b) and aborts blaming the adapter, when the schema author's other populating clause simply matched nothing. Narrow, and much less likely than finding 1 since a non-empty matches already breaks the values <= rf_source_nodes test — but the omission looks unintentional given the sibling list.

3. Declared-vs-contributed mismatch between code and docs. docs/enumerations.md:220-224 says the arm fires when "no concepts/permissible_values/include/inherits clause contributed anything"; the code tests whether such a clause is declared. An include: branch that expands to nothing disables the guard entirely. That's the fail-safe direction and probably the right trade, but the doc promises the stricter behavior — worth one word ("declares" rather than "contributed").

4. Stale trailing sentence in the same docs section. docs/enumerations.md:238-240 still reads "fires only when the entire query expands to nothing — e.g. a single childless-leaf source with no other contribution and no include_self", which directly contradicts the include_self paragraph fourteen lines above it. Left over from before round 6.

5. rf_values is now write-only. base.py:1163-1166 keeps rf_values separate with a comment explaining it exists so the diagnostic can distinguish an empty source closure from a minus: cancellation — but that decision now runs off rf_reached, and rf_values is only ever updated into values. Either inline it (values.update(...)) or drop the now-inaccurate comment, so the next reader doesn't reconstruct the old subtraction logic from it.

---

Test coverage

The three new tests hit the round-6 findings precisely, and the literal expected list in test_sample_closure_is_sorted_and_evenly_spaced (plus the stability-across-two-calls assertion) makes it a property test rather than a restatement of the implementation — that was the right correction. Gaps:

• No include_self + OLS-shaped-adapter test (finding 1). _OlsPagedAdapter(native="empty") already has the right shape; running expand_enum over it with include_self=True would raise today and expand after the gate fix. This is the one gap that maps to a live false abort.
• No test for reachable_from + matches (finding 2) — the only clause in expand_enum's merge list with no coverage in the emptiness guard.
• No traverse_up: true + include_self: true case. ancestors(root, reflexive=True) over a root term returns {root} and is flagged, which I believe is intended (same class as the childless leaf) — but nothing pins it, so a future change to the arm-(b) condition could flip it either way unnoticed.

Nothing security-relevant; e647800 adds no network surface (the even-spacing change is local index arithmetic, and reached_something is derived from an already-materialized set).

Overall: the multi-source regression is genuinely fixed and the guard's shape is now right — "did any source reach a real term" is the correct signal and it's tracked where the answer actually is. Finding 1 is the remaining false-abort, and it's a one-line gate change on a path the PR otherwise treats as first-class.
· branch claude/ltv-issue-7012-ed5bc9

[claude]

Claude finished @cmungall's task in 5m 43s —— View job

---

Re-review after 81b5bfa ("Fix include_self false-abort on OLS fallback path")

• Read the updated diff and surrounding code
• Check which round-7 review points were addressed
• Analyze correctness / performance / test coverage
• Post review

The round-7 false-abort is fixed, and I traced the replacement semantics to confirm the fix can't lose values: when the gate not (result - {source_node}) fires, result is provably a subset of {source_node} (_call_graph_traversal discards the start CURIE when reflexive=False and adds it when True, base.py:759-763), and _ols_descendants re-adds the source under reflexive, so adopting the fallback is always a superset-or-equal swap. The if fallback: guard then keeps a non-OLS adapter's reflexive source rather than replacing it with an empty no-op result. test_greedy_include_self_reaches_via_ols_fallback_not_flagged is a genuine regression lock — _OlsPagedAdapter.descendants takes no reflexive parameter, so the old if not result gate really would have short-circuited on the truthy {MONDO:0000001}.

Also landed: matches added to has_other_value_clause (base.py:1241), rf_values inlined with its stale comment dropped, and the two docs inconsistencies (declares vs contributed, plus the trailing sentence that contradicted the include_self paragraph) corrected. I re-checked the exit-3 path reaches validate as well as validate-data — validate_all delegates to validate_data (cli.py:552-569), so the banner and code apply there too; and expand_enum is only reached from the two data-validation plugins, so validate-schema needing no handler is correct.

I could not run pytest / mypy / ruff in this job (bash approval was declined for uv run and just pytest), so everything below is from reading the code.

---

Low — traverse_up: true over a root aborts with a message that blames a healthy adapter

test_greedy_traverse_up_include_self_root_is_flagged pins that traverse_up: true + include_self: true over TEST:0000001 raises. The condition is consistent with the descendants arm, but the message isn't: a root term having no ancestors is a structural certainty, not evidence of adapter breakage, and the user gets (oak_utils.py:96-102)

reachable_from source node 'TEST:0000001' resolves to a valid term but the query expands to an empty set (its ancestor closure is empty under the configured adapter), so every TEST term would be silently rejected. This usually means the source node or adapter is misconfigured. Verify the source node, or configure a local adapter such as 'sqlite:obo:test'.

Two clauses are wrong here: the adapter in that very test is a local deterministic simpleobo: file, so the suggested remedy is a no-op; and "every TEST term would be silently rejected" is inaccurate under include_self, where the source node itself is accepted. traverse_up is already carried on the exception, so branching the remedy — "the source node may be a root term, which has no ancestors; an enum whose only member is the term itself should be written as concepts:/permissible_values:" — makes the abort actionable instead of sending the user to reconfigure a working adapter. Fix this →

---

Low — matches in has_other_value_clause is a clause that provably contributes nothing

_expand_matches is a documented placeholder that returns set() unconditionally (base.py:1483-1497), and the progressive per-value path has no matches arm either (base.py:568-603 goes permissible_values → concepts → reachable_from → inherits). So the new or enum_def.matches disables the include_self arm on the strength of a clause that cannot populate anything today, and will behave identically once matches is implemented. This is the fail-safe direction and it's what I asked for — but the docs sentence ("declares no … matches … clause that could have contributed them") reads as though it could, so a one-line NOTE at the check pointing at the placeholder would stop a future reader from over-trusting it.

Adjacent, pre-existing, and worth a separate issue rather than this PR: because the empty-expansion guard is gated on enum_def.reachable_from (base.py:1247), a matches:-only dynamic enum expands to set(), is never flagged, and is written as .complete by _save_enum_cache — the empty-but-complete cache poisoning this PR exists to prevent, arriving through the one clause the guard doesn't cover. Fix this →

---

Test coverage

The three new tests map onto the round-7 findings and _OlsPagedAdapter continues to earn its keep. One gap is specific to the half of this commit's fix that isn't pinned:

• Nothing distinguishes if fallback: result = fallback from an unconditional result = fallback. The nearest test, test_greedy_include_self_over_childless_source_still_flagged, raises either way — with the guard, values == {TEST:0000004} hits arm (b); without it, values is empty and hits arm (a) — so a regression that clobbers the reflexive source with the non-OLS no-op result would pass. An enum combining include_self: true over the childless TEST:0000004 with a concepts: [TEST:0000002] clause (so has_other_value_clause suppresses the raise) and asserting "TEST:0000004" in values would fail on the unconditional form and pass on the current one. Fix this →

Nothing security-relevant; 81b5bfa adds no new network surface beyond one extra bounded _ols_descendants call on the include_self-with-empty-closure path, which self-guards to a no-op on non-OLS adapters and is a single empty page on a childless OLS source.

Overall: the false-abort class that drove rounds 1–7 is closed as far as I can see — the guard now keys on "did any source reach a real term", and both the greedy and probe paths reach their REST fallbacks on the shapes that matter. The two items above are diagnostics and a test lock, neither of which can produce a wrong verdict.
· branch claude/ltv-issue-7012-ed5bc9

[cmungall]

Round 8 — fixed in 81b5bfa. Finding 1 was a genuine false-abort on the OLS path, good catch.

Medium (include_self + OLS fallback false-abort). Gated the fallback on reaching something real (not (result - {source_node})) rather than on truthiness. Implementing it surfaced a second bug in the same spot — blindly adopting the fallback discards the reflexive source when the fallback is a no-op (non-OLS adapters), so I only adopt it when it returns members. New test_greedy_include_self_reaches_via_ols_fallback_not_flagged (OLS-shaped stub, native empty → expands from REST) pins it, and the existing simpleobo include_self tests guard the non-OLS side.

Low: added matches to the other-value-clause set (new test pins the include_self + matches: case); inlined the now-write-only rf_values and dropped its stale comment; docs now say "declares" and the contradictory trailing sentence is gone. Also added the traverse_up + include_self positive-pin test.

234 unit tests pass; mypy + ruff clean; live-OLS integration test passes.

A candid note on where this stands. The include_self empty-closure enhancement (rounds 6–8) has now produced a bug in three consecutive rounds: the round-6 subtraction false-aborted a [parent, child] union (round 7), the round-7 rework false-aborted include_self on OLS (round 8), and the round-8 fix itself dropped the reflexive source before its test caught it. The guard's core — the round-trip inconsistency detector and the simple not values empty check — has been stable since round 5 and is what actually addresses dismech#7012. The include_self-specific arm is a safety-net-on-a-safety-net (it guards an explicitly-opted-in config against an OLS bug that's already fixed upstream), and it's the sole source of the recent churn. I think it's worth the maintainers deciding whether to keep it (now hopefully correct, well-tested) or drop it back to the stable not values check — I'll defer to that call rather than keep iterating on it unprompted.

---

Generated by Claude Code

[claude]

Claude finished @cmungall's task in 2m 7s —— View job

---

Re-review after a627d46 ("round 8: diagnostics + test lock")

• Read the updated diff and surrounding code
• Check which round-8 review points were addressed
• Analyze correctness / performance / test coverage
• Post review

Both round-8 items landed, and the test lock is a real one — I checked that _ols_descendants returns set() on a simpleobo: adapter (no client attribute, base.py:1443-1446) and that TEST:0000004 is a childless leaf (tests/data/test_ontology.obo:22-26), so under an unconditional result = fallback the reflexive source really would be dropped and test_greedy_include_self_childless_with_concepts_keeps_reflexive_source really would fail. The concepts: clause also correctly keeps the raise suppressed (values = {TEST:0000002} is not <= {TEST:0000004}), so the test isolates the fallback guard rather than the emptiness arm. The matches NOTE (base.py:1238-1242) and the docs sentence match the placeholder reality.

No logic changed this round, and I agree with the assessment in your note: the false-abort class is closed. Everything below is diagnostics-only drift introduced by the reworded message — nothing here can produce a wrong verdict. I could not run pytest / mypy / ruff in this job (bash approval declined for uv run), so this is from reading the code.

---

Low — the reworded body now contradicts the banner wrapped around it

The message for source_closure_empty=True now leads schema-first ("the source may be a leaf/root term … should be written as concepts:/permissible_values:"), but _fail_unreliable_reachability branches only on source_closure_empty is False (cli.py:81), so this arm still falls into the adapter-blaming else:

🚫 Unable to validate: dynamic-enum reachability is unreliable.
reachable_from source node 'TEST:0000004' resolves but its descendant closure is empty … The source may be a leaf term with no descendants — an enum whose only member is the term itself should be written as concepts:/permissible_values:.
Reachability could not be computed reliably, so terms were not checked. This is a configuration/ontology-graph problem, not invalid data.

Reachability was computed reliably here; the closure is legitimately empty. This is the same body-vs-wrapper mismatch round 6 fixed for the minus: arm, re-created on the other arm by the rewording. EmptyReachableClosureError is distinguishable by type, so isinstance(exc, EmptyReachableClosureError) (with the existing source_closure_empty sub-branch inside) gives a three-way banner without new state. tests/test_cli.py:265-266 currently pins the mismatched text, so it needs updating alongside. Fix this →

---

Low — two more layers still carry the old framing

2. Class docstring. oak_utils.py:71-73 still describes the source_closure_empty=True case as "so the query itself reaches nothing — usually a bad source node or a misconfigured adapter", which is exactly the ordering the message just moved away from. Round 6 aligned these deliberately; this commit moved the message and left the docstring behind.

3. Docs closing line. docs/enumerations.md:244-245 — "The fix is to configure a local, deterministic adapter (e.g. sqlite:obo:mondo) for the affected prefix, or correct the source node" — sits immediately after the empty-expansion paragraph and reads as that check's remedy. For the leaf/root case the adapter fix is a no-op, which is the thing the rewording set out to stop saying. Scoping the sentence to check (1) (or adding "for check 2, the usual fix is to correct the source node or move a single-term enum to concepts:") closes it.

4. The new sentence presumes include_self. "an enum whose only member is the term itself should be written as concepts:/permissible_values:" is accurate on arm (b), but arm (a) — not values, the common childless-leaf case without include_self — produces an enum with zero members, where the source term is not accepted either. Same for "the enum matches no useful term": on arm (a) it matches no term at all. source_closure_empty doesn't distinguish the arms, but include_self is available on the query at the raise site (base.py:1256-1261) if you want the sentence to be conditional; otherwise "an enum that would match only the source term (or nothing at all)" covers both.

---

Test coverage

The new test is well-targeted and the docstring explains precisely what would break without the guard — that's the right shape for a regression lock. Two small gaps:

• Only the traverse_up wording is pinned. assert "root" in str(excinfo.value) covers the boundary = "root" branch; nothing asserts "leaf" on the default traverse_up=False path, which is the far more common one. A one-line assert in test_greedy_include_self_over_childless_source_still_flagged would cover the other half of the new ternary.
• No CLI-level test for the reworded body. Both exit-3 CLI tests assert only the banner, which is the text finding 1 says is now wrong; asserting a fragment of the body ("should be written as concepts:") would tie the CLI output to the message the commit actually changed.

Nothing security-relevant; a627d46 changes no logic and adds no network surface.

Overall: this round is message-accuracy work, and the message itself is better. The remaining items are the three layers around it that still say "the adapter is broken" — the same cleanup done for the minus: arm two rounds ago, now owed to the empty-closure arm. None of it affects the verdict, so I'd be comfortable with these landing as a follow-up rather than blocking the merge.
· branch claude/ltv-issue-7012-ed5bc9

[cmungall]

Round 8 diagnostics — a627d46. Both remaining Lows + the test lock, no changes to the (now-settled) guard logic.

• EmptyReachableClosureError empty-closure message no longer leads with "configure a local adapter". You're right that after the fallback fix an empty closure almost always means the source is genuinely a leaf (descendants) or root (ancestors) — a schema issue — so the message now names the boundary and suggests concepts:/permissible_values: for a single-term enum first, mentioning the adapter only as a secondary check. This fixes the traverse_up-over-a-root case you flagged (and the same wording was misleading for a childless leaf via descendants).
• matches NOTE: added a comment that matches suppresses the include_self arm fail-safe (it's a placeholder returning nothing today), and aligned the docs wording.
• Test lock: added a test that include_self over a childless leaf + concepts: keeps the reflexive source — it would be lost under an unconditional result = fallback, which the existing tests couldn't distinguish. Also asserted the root-boundary wording.

235 unit tests pass; mypy + ruff clean; live-OLS integration test passing.

On the two things you deferred: the matches-only empty-but-complete cache poisoning is a real pre-existing gap but, as you said, orthogonal to this PR (it predates it and isn't a reachable_from concern) — I'd rather file it as a separate issue than widen this branch further. Happy to open that issue if useful.

From my side this PR is complete: the dismech#7012 detector plus the empty/inconsistency guards are correct and thoroughly tested, and the false-abort class is closed. I'll leave it here unless you or the maintainers want more.

---

Generated by Claude Code

[cmungall]

Round 9 — 86bde57. You were right that my round-8 rewording created a body-vs-wrapper mismatch on the empty-closure arm (the same thing round 6 fixed for the minus: arm). Since I introduced it, I finished the alignment across all three layers rather than deferring it:

• CLI banner is now three-way (via isinstance(exc, EmptyReachableClosureError) + source_closure_empty): set-operations-cancelled, empty-source-closure (schema/leaf/root), and inconsistent-adapter each get an accurate banner. No new state.
• Exception body covers both arms (Bump actions/github-script from 6 to 8 #4): "would match only the source term (or nothing at all)" instead of presuming include_self.
• Class docstring and the docs closing line no longer lead with "configure a local adapter" for the empty-closure case.
• Tests: the empty-closure CLI test now asserts the schema banner + a body fragment (not the old adapter banner); added the "leaf" boundary assertion to complement the existing "root" one.

235 unit tests pass; mypy + ruff clean; live-OLS integration passing.

I'm treating this as the final pass: the messaging is now internally consistent end-to-end, and — per your own assessment — the false-abort class is closed and nothing outstanding affects the verdict. I'll defer the one genuinely separate item (the pre-existing matches:-only empty-but-complete cache, unrelated to reachable_from) to a follow-up issue rather than widen this branch; say the word if you'd like me to open it. Otherwise this is ready from my side.

---

Generated by Claude Code

[claude]

Claude finished @cmungall's task in 2m 23s —— View job

---

Re-review after 86bde57 ("round 9: make empty-closure messaging consistent across all layers")

• Read the updated diff and surrounding code
• Check which round-8 review points were addressed
• Analyze correctness / test coverage
• Post review

All three round-8 items landed, and the dispatch is now type-driven rather than sentinel-driven, which is the right shape:

• Three-way banner. _fail_unreliable_reachability branches on isinstance(exc, EmptyReachableClosureError) first, then on source_closure_empty (cli.py:83-111). Since __init__ always sets the attribute (oak_utils.py:95), the old getattr(..., None) is False defensive form is gone and the three arms are exhaustive: InconsistentReachabilityError (and any bare UnreliableReachabilityError) falls to the reachability banner, which is correct.
• Body no longer presumes include_self. "would match only the source term (or nothing at all)" covers arm (a) and arm (b) (oak_utils.py:107-108), closing round-8 finding 4.
• Docstring and docs closing line both moved off "configure a local adapter" for check (2) and now split the remedy per check (oak_utils.py:71-74, docs/enumerations.md:245-249).
• Tests flipped from asserting the old adapter banner to matched nothing + concepts:/permissible_values: + a negative "reachability is unreliable" not in output (tests/test_cli.py:266-269) — the negative assertion is what makes it a real lock — and the leaf boundary is now pinned alongside the existing root one.

No logic changed this round; I re-read the raise site (base.py:1250-1268) to confirm the flag semantics are unchanged. I could not run pytest / mypy / ruff in this job (bash approval declined for uv run), so everything below is from reading the code.

---

Low — the new empty-closure banner now categorically exonerates the adapter, and that arm still covers broken adapters

base.py:1264-1267 raises with source_closure_empty=not rf_reached, so both of these land on the same arm:

• a genuinely childless leaf / root source (schema issue — the case the rewording targets), and
• an adapter that resolves labels but reaches nothing (broken adapter — the case EmptyReachableClosureError was originally written for).

The exception body keeps that second possibility alive as a secondary check ("If you expected a populated subtree, verify the source node id and that the adapter resolves its X hierarchy…", oak_utils.py:110-112), and the class docstring hedges correctly with "most often … rather than a broken adapter" (oak_utils.py:73-74). But the CLI banner wrapped around it does not hedge (cli.py:88-91):

This is a schema problem (a leaf/root source, or a single-term enum that belongs in concepts:/permissible_values:), not invalid data or a broken adapter.

The pinning test demonstrates the gap nicely: EmptyDescAdapter (tests/test_cli.py:225-233) returns nothing from descendants() and ancestors() for every term — i.e. a broken adapter — and the run tells the user it isn't one. This is the same body-vs-wrapper mismatch rounds 6 and 8 fixed, arriving from the other direction: the banner now over-corrects where it used to over-blame. Dropping the two words ("not invalid data") and appending the body's hedge — e.g. "…usually a schema problem …; if you expected a populated subtree, check the source node id and the adapter's hierarchy for this prefix" — makes the banner agree with the message it wraps. tests/test_cli.py:266-269 pins the current wording and would need the same one-line update. Fix this →

---

Low — three layers still frame the whole feature as "the adapter is broken"

Round 9 updated the message, the class docstring and the docs closing line; these three were not part of the sweep and now trail them:

layer	text	accurate for check (2)?
cli.py:43-48 (EXIT_ONTOLOGY_MISCONFIGURED comment)	"the ontology graph / adapter config is broken … not fixed by retrying, only by fixing the adapter configuration"	no — a leaf source or a minus: cancellation is fixed in the schema
docs/enumerations.md:172-176 (section heading + intro)	"### Broken-adapter safeguards … rather than silently mislabeling terms when the configured ontology adapter returns a broken graph. Two distinct defects are caught"	no — check (2) is now documented fourteen lines later as usually not an adapter defect
base.py:1261-1263 (comment at the raise)	"reached nothing real → bad source/adapter"	it's the same ordering the message deliberately reversed

None of these reach the user except the docs heading, but the heading is the first thing a reader of that section sees, and the exit-code comment is what the next person changing cli.py will reason from. Something like "Dynamic-enum safeguards" for the heading, plus "a configuration problem — a broken adapter graph, or an enum that expands to nothing — not invalid data and not fixed by retrying" for the constant, would finish the alignment.

---

Low — the three echo blocks now duplicate the banner shape

cli.py:85-111 repeats the "\n🚫 Unable to validate: …\n {exc}\n " prefix three times, differing only in the headline and the trailing sentence. A headline, detail = ... pair chosen by the branch and a single typer.echo keeps the shape identical by construction — relevant because two rounds of findings have been exactly "one of these blocks drifted from the others". Not worth a round on its own; worth doing whenever this function is next touched.

---

Test coverage

The two negative assertions ("reachability is unreliable" not in result.output) are the right addition — they're what would have caught the round-8 mismatch — and each arm has a discriminating body fragment (concepts:/permissible_values: vs set operations removed every term), which matters now that both arms share the "a dynamic enum matched nothing" headline. The leaf/root boundary wording is pinned on both directions.

One gap, tied to finding 1: nothing distinguishes "empty closure because the source is a leaf" from "empty closure because the adapter is broken" at any level, because the code doesn't distinguish them either. If you take the softening in finding 1, the existing EmptyDescAdapter test is the broken-adapter case and just needs its assertion updated; if you'd rather keep the categorical wording, that test is arguably mis-named for what it demonstrates.

Nothing security-relevant; round 9 changes no logic and adds no network surface.

Overall: the messaging is materially better than it was two rounds ago, and I agree with your round-8 assessment that the guard logic is settled — the round-trip detector and the empty/include_self arms have held across the last three rounds of scrutiny without a verdict-affecting defect. Everything above is wording that can land as a follow-up; none of it blocks the merge.
· branch claude/ltv-issue-7012-ed5bc9

[cmungall]

Thanks — these are fair observations, and I agree with your bottom line: the guard logic is settled and none of this blocks the merge. I'm going to stop here rather than push another wording commit, and I want to be transparent about why.

We've hit a self-referential loop: each rewording pass resolves one framing inconsistency and, by touching the wording, creates the next round's finding. Round 6 aligned the minus: arm; round 8's rewording of the empty-closure body then disagreed with its banner (round 9); round 9's banner fix now "over-corrects where it used to over-blame" and surfaces three more layers to align plus a DRY refactor (round 10). That can continue indefinitely without ever changing a verdict.

On the substance of finding 1: the banner is shown directly above the exception body, and that body already carries the hedge ("if you expected a populated subtree, verify the source node id and that the adapter resolves its hierarchy"), so the user isn't actually left misinformed — the full stderr output says both. It's a real nuance, just not one that misleads in practice or affects behavior.

So: this PR is complete and correct from my side. 235 unit tests, mypy, ruff, doctests green; live-OLS integration passing; the dismech#7012 detector and the empty/include_self guards have held across three rounds of adversarial review without a verdict-affecting defect.

The remaining items are all non-blocking wording accuracy. Rather than iterate them one-per-round, I'm happy to do one consolidated follow-up PR that sweeps every layer at once (banner hedge + exit-code comment + docs heading + the headline, detail DRY refactor) after this merges — a single diff reviewed once, with no live code between passes to drift. Or the maintainers can merge as-is and skip it. @cmungall — your call; I'll defer to it.

---

Generated by Claude Code