Fix crashes and mismatches in PhylorefWrapper and PhylogenyWrapper on edge-case input by gaurav · Pull Request #196 · phyloref/phyx.js

gaurav · 2026-10-14T05:35:24Z

This PR fixes edge-case bugs in PhylorefWrapper and PhylogenyWrapper that came up while reviewing #195. Most are in methods that #195 published in the API docs, whose doc comments describe what each method is meant to do; this PR makes the code do it. Two of them threw on input the format allows, one matched the wrong node, and two dropped information from the OWL and JSON-LD output.

The call worth overruling

A phylogeny's default nomenclatural code is filled in where getTaxonomicUnitsForNodeLabel() returns a node's explicit taxonomic units, on a copy, rather than only where getNodesAsJSONLD() builds the node's OWL restriction. So the restriction, the representsTaxonomicUnits copied into the same node, and every other caller all agree. A taxon concept with only a nameString gets its name parsed with the default, as TaxonConceptWrapper already does when reading it. This changes OWL and JSON-LD output only for phylogenies with a default code whose additionalNodeProperties list taxon concepts without one. None of the example fixtures do, and no expected output changed. Taxonomic-unit matching doesn't look at codes, so it is unaffected.

What's fixed

setSpecifierType(specifier, 'External') threw Cannot read properties of undefined on a phyloref missing either specifier list. It now creates only the list it adds to, and leaves the phyloref without an empty list for the other side.
getExpectedNodeLabels() called includes() on expectedPhyloreferenceNamed, which is a substring test when that property is a single name, the form our own tests use. So phyloref1 matched a node expecting phyloref10. A single name is now wrapped in a list first, and a phyloref with no label matches no node.
getParsedNewick() set name from label while walking the arcs, and a single-node tree has none. So a one-taxon phylogeny exported a node with no labels and no taxonomic units. Every node is now named before the walk.
getNodesAsJSONLD() typed a node's taxonomic units without the phylogeny's default nomenclatural code; see above.

TaxonomicUnitWrapper also defaulted to owlterms.NAME_IN_UNKNOWN_CODE, which owlterms doesn't define. It now uses UNKNOWN_CODE like every other wrapper. Both are undefined, so its behaviour doesn't change.

Verified

CI passes on Node 22, 24 and 26 at e577d4e, as do the full suite and lint locally under Node 26.13.0 with JPhyloRef under Java 21; the one pending test is the long-standing placeholder for brochu_2003 normalization phylogenies. With src/ reset to the base branch, every new or changed test fails except one: the list form of expectedPhyloreferenceNamed, which already worked.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…lists The 'External' branch read this.phyloref.internalSpecifiers and externalSpecifiers directly, so on a phyloref missing either list it threw "Cannot read properties of undefined (reading 'indexOf')". The 'Internal' branch went through the getters, which create a missing list, and so worked. Use the getters in both. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

getExpectedNodeLabels() called includes() on expectedPhyloreferenceNamed, which is a substring test when that is a string rather than a list -- the form our own tests use. So a phyloref labelled 'phyloref1' was reported as expected to resolve to a node expecting 'phyloref10'. Wrap a single name in a list first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

getCurrentStatus() read pso:withStatus off the last pso:holdsStatusInTime entry without checking it was there, so an entry without one threw. getStatusChanges() already allows for such entries; fall through to the draft default instead, as for a phyloref with no entries at all. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

getParsedNewick() copies each node's label to `name` while walking the arcs, and a tree with a single node has none, so its node never got a name. getNodesAsJSONLD() reads labels from `name`, so a one-taxon phylogeny exported a node with no labels and no taxonomic units. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…c units getNodesAsJSONLD() wrapped each of a node's representsTaxonomicUnits without the phylogeny's default nomenclatural code, so a taxon concept that set no code of its own was typed with none, and could fail to match a specifier built with that default. The `if (wrappedTUnit)` around it was always true; drop it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

It defaulted to owlterms.NAME_IN_UNKNOWN_CODE, which owlterms doesn't define. That is `undefined` as well, so nothing changes, but every other wrapper uses UNKNOWN_CODE and a reader shouldn't have to check that the two agree. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Wrapping expectedPhyloreferenceNamed in a list turned a missing value into [undefined], which includes() then matched against an unlabelled phyloref's undefined label. Before that change this threw. A phyloref without a label can't be named by a node at all, so return no expected nodes for it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…g test The substring test relied on phylogeny1, defined far above, happening to use a single string; the list test next to it read as if it guarded the same bug, but includes() on a list was never a substring match. Give the substring test its own phylogeny with the string form spelled out, and say what the list test doesn't cover. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@id

…d none The previous fix made getCurrentStatus() fall back to 'pso:draft' when the last pso:holdsStatusInTime entry had no pso:withStatus, so appending an entry with only a time interval demoted a published phyloref to a draft. Use the last entry that records a status instead. Its guard also only checked that pso:withStatus was present, so a null one, or one without an @id, still threw (or gave an undefined status) in both getCurrentStatus() and getStatusChanges(). Treat those as recording no status. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

JSON-LD lets a single value be written without a list around it. For such a phyloref getCurrentStatus() quietly reported a draft, getStatusChanges() threw ".map is not a function", and setStatus() threw on push(). Read it as a one-entry list in all three; setStatus() replaces it with that list so it can append. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

setSpecifierType() removed the specifier from the other list through its getter, which creates that list if it's missing, so making a specifier external on a phyloref with no lists also wrote `internalSpecifiers: []` onto the caller's object (and vice versa), and it was serialized from there. Only look in the other list if the phyloref has one; still create the list we add to. The tests checked through the same getters, so they couldn't see this; check the phyloref object instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

getParsedNewick() set `name` from `label` on each arc's parent and child, and then again on the root, which a single-node tree's lack of arcs left unnamed. Name every vertex once, before the arc loop, so there's one place to change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…mic units The earlier fix passed the default code only to the wrapper that builds a node's OWL restriction, so getNodesAsJSONLD() typed the node with ICZN (say) while the representsTaxonomicUnits it copied into the same node had no code, and getTaxonomicUnitsForNodeLabel() returned no code either. Fill the default in where getTaxonomicUnitsForNodeLabel() returns explicit taxonomic units, on a copy, so every caller sees the same code. A taxon concept with only a nameString gets its name parsed with the default, as TaxonConceptWrapper does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot

🟡 Changes recommended

Status updates can leave the previous status interval open when a statusless entry follows it.

1 open finding

setStatus closes the wrong entry after an interval-only record · New

What changed in this PR

Fixes wrapper edge cases so Phyx processing matches the API behavior documented in #195.

Changes:

Handles missing specifier lists, exact expected-name matching, and incomplete status histories.
Preserves single-node labels and applies default nomenclatural codes consistently.
Adds regression tests and a changelog entry.

File	Description
test/phylorefs.js	Tests specifier, name-matching, and status edge cases.
test/phylogenies.js	Tests single-node labels and default nomenclatural codes.
src/wrappers/TaxonomicUnitWrapper.js	Uses the correct unknown-code constant.
src/wrappers/PhylorefWrapper.js	Improves specifier, matching, and status handling.
src/wrappers/PhylogenyWrapper.js	Preserves node labels and propagates default codes.
CHANGELOG.md	Records the fixes.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…ntries getCurrentStatus() now skips pso:holdsStatusInTime entries that record no status, but setStatus() still ends the interval of the last entry, whatever it holds. Copilot flagged this on PR #196. Status tracking isn't used by any current tool, and setStatus() never writes a statusless entry, so the behaviour is left as it was; the comment says what to change if the feature comes back. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

hlapp

See inline comment.

… one has no doc comment (#195) The published page for `PhylogenyWrapper` was missing ten of its twelve members. They were described, often at length, but with `//` comments inside the method body, which jsdoc never reads: it publishes a member only when a `/** */` comment sits directly above it. The same survey over `src/` found twelve more in `PhylorefWrapper` (two of them under a block comment that opened `/*` instead of `/**`), one in `PhyxWrapper` and five in `TaxonNameWrapper`. This PR moves that prose above each member so all 28 appear on the site, and adds a test so the next one cannot go missing silently. ## The calls worth overruling **The comments are moved, not rewritten.** Each doc comment is the original author's text, with argument lists turned into `@param` and `@return` tags in the style `PhyxWrapper.asJSONLD` already uses. Line-level implementation notes stay in the bodies. Where the text was wrong it is corrected: `PhylorefWrapper.asJSONLD` said "phylogeny", `getCurrentStatus` described a `status` key the code returns as `statusCURIE` (and a comment in its body said it read the first status entry rather than the last), `PhyxWrapper.defaultNomenCode` described a fallback the code doesn't have, and `getParsedNewick` and `getNodesAsJSONLD` described properties every node has that only some nodes do. An unknown nomenclatural code is typed as `undefined`, because `owlterms.UNKNOWN_CODE` is, not as a string or an IRI. **Nothing is marked `@private`.** Klados calls eight of these methods directly, including `recurseNodes` and `getParsedNewickWithIRIs`. The three OWL-building helpers in `PhylorefWrapper` are only called internally, but `createComponentClass` was already published, so its two siblings are published the same way. Hiding them would be a separate decision. **The check asks jsdoc, rather than reading the source itself.** It runs `jsdoc --configure jsdoc.config.js --explain`, which parses `src/` with the build's own config in well under a second and builds nothing, and fails on any class member whose doclet has no comment. So the files checked and the comments that count are exactly the ones `npm run docs` uses. I passed over matching lines of source: that approach is fooled by a `}` at the start of a line in a template literal, a trailing `/* */` after the previous member, a `//` lint directive between the comment and the member, and `/**/`, and its file list ignored `excludePattern`. [docs/Documentation.md](https://github.com/phyloref/phyx.js/blob/1776f5a/docs/Documentation.md#validating-the-docs) records this so nobody goes back to it. The sidebar-navigation test now takes its file list from jsdoc too. ## What's here - Doc comments on every previously unpublished member of `PhylogenyWrapper`, `PhylorefWrapper`, `PhyxWrapper` and `TaxonNameWrapper`, one commit per file, with every `PhylogenyWrapper` method now carrying `@param` and `@return`. - `test/docs.mjs`: every class member jsdoc finds must have a doc comment. It also now loads `owlterms.js` and `jsdoc.config.js` with `require()` instead of spawning `node -p` for each. - Two new entries in the [Gotchas](https://github.com/phyloref/phyx.js/blob/1776f5a/docs/Documentation.md#gotchas): the comment trap itself (`//`, `/* */`, `/***` and `/**/` are all skipped, as is a `/**` with another block comment between it and the member), and that the theme drops a bulleted list inside a `@param` or `@return` tag, so lists go in the description. ## Verified On 1776f5a, with Node 26 and Java 21, `npm run docs` builds and the full suite passes with lint clean (the one pending test is an unimplemented placeholder in the normalization examples). Run against `master`'s sources, the new check names exactly the 28 members fixed here; on a sample file it also catches each of the four cases above. With clean-jsdoc-theme 5.2.0 a list inside `@param` or `@return` is still dropped, so that gotcha still holds. ## Before merging - [ ] **Blocking:** four of the new comments describe the intended behaviour of code that gets edge cases wrong today. `setSpecifierType(…, 'External')` throws on a phyloref with no specifier lists. A single `expectedPhyloreferenceNamed` is matched as a substring. `getCurrentStatus` throws on a status entry with no status. `getParsedNewick` leaves the node of a single-node tree unnamed. #196, stacked on this branch, fixes all four with regression tests. Merge it with this PR or right after it, or these docs misdescribe what shipped. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

…ylorefWrapper (#198) `PhylorefWrapper` carried a small publishing workflow: `getCurrentStatus()`, `getStatusChanges()`, `setStatus()` and the static `getStatusCURIEsInEnglish()` recorded a history of Publishing Status Ontology states (draft, final draft, under review, tested, published, retracted) under `pso:holdsStatusInTime` on each phyloreference. We have since decided that was more machinery than the problem needed: curators can track where a phyloreference stands in curator notes, or by filing Phyx files in a directory per status, and the one case with real semantics, deprecation, belongs in standard OWL as `owl:deprecated`. [Klados](https://github.com/phyloref/klados) has already dropped its status editor and does not call these methods. This PR removes the four methods, their tests, and the `moment` dependency they were the only user of. It is the first half of #135. Hilmar's [review of #196](#196 (comment)) prompted this: that PR adds fixes to these same methods, which is why the leftover code came up. **The two PRs conflict in `src/wrappers/PhylorefWrapper.js`.** If this one merges first, #196 should drop its status-method fixes, their tests, and the matching sentence in its changelog line, since there is nothing left to fix. If #196 merges first, this branch needs a rebase that deletes the fixed code as well. ## What this deliberately does not do - The second half of #135, a single `owl:deprecated` marker in place of `pso:retracted-from-publication` (klados#81), is a separate PR stacked on this one, so #135 stays open. - The `pso`, `tvc` and `timeinterval` prefixes stay in every JSON-LD context under `context/`, including `development`. Older Phyx files still carry `pso:holdsStatusInTime` entries (two papers in clade-ontology, and our own `test/examples/incorrect/otl-resolution-errors.json`), and those keep expanding to full IRIs rather than becoming undefined terms. The library still reads such files; it just no longer interprets that property. ## Verification `npm test` passes on 0d28b7d: all 210-odd tests, lint clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Master (#198) removed PhylorefWrapper's status methods, which this branch had fixed (single pso:holdsStatusInTime entries, entries with no status). The conflicts are resolved in master's favour: the status methods, their tests and the getStatusesInTime()/getStatusCURIE() helpers this branch added are gone, and the #196 changelog entry no longer mentions them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

hlapp

👍🏻

gaurav changed the title ~~Fix setSpecifierType, getExpectedNodeLabels, getCurrentStatus and single-node phylogenies on edge-case input~~ Fix crashes and mismatches in PhylorefWrapper and PhylogenyWrapper on edge-case input Oct 14, 2026

gaurav added a commit that referenced this pull request Oct 14, 2026

Add PR #196 to the changelog

2e542f5

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

gaurav added a commit that referenced this pull request Oct 14, 2026

Add PR #196 to the changelog

ef02534

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

gaurav force-pushed the fix-wrapper-edge-cases branch from 2e542f5 to ef02534 Compare October 14, 2026 07:20

This was referenced Oct 14, 2026

Publish every wrapper method in the API docs, and fail the tests when one has no doc comment #195

Merged

Add @return, @param and @type tags to the rest of the published API #197

Open

gaurav and others added 7 commits October 14, 2026 03:30

Add PR #196 to the changelog

cd21866

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

gaurav force-pushed the fix-wrapper-edge-cases branch from ef02534 to cd21866 Compare October 14, 2026 07:31

gaurav and others added 8 commits October 14, 2026 11:49

Describe PR #196's review fixes in the changelog

c8d6c51

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

gaurav force-pushed the fix-wrapper-edge-cases branch from 0157e9f to c8d6c51 Compare October 14, 2026 16:08

gaurav requested a balanced review from Copilot October 14, 2026 16:17

Copilot started reviewing on behalf of gaurav October 14, 2026 16:18

View session

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Comment thread src/wrappers/PhylorefWrapper.js Resolved

gaurav requested a review from hlapp October 14, 2026 16:59

hlapp requested changes Oct 28, 2026

View reviewed changes

Comment thread src/wrappers/PhylorefWrapper.js Resolved

Base automatically changed from document-wrapper-methods to master October 28, 2026 08:07

gaurav mentioned this pull request Nov 3, 2026

Remove phyloreference status tracking (pso:holdsStatusInTime) from PhylorefWrapper #198

Merged

gaurav requested a review from hlapp November 9, 2026 07:27

hlapp approved these changes Nov 9, 2026

View reviewed changes

gaurav merged commit a75bd92 into master Nov 10, 2026
3 checks passed

gaurav deleted the fix-wrapper-edge-cases branch November 10, 2026 04:00

Navigation Menu

Repository navigation

Fix crashes and mismatches in PhylorefWrapper and PhylogenyWrapper on edge-case input - #196

Fix crashes and mismatches in PhylorefWrapper and PhylogenyWrapper on edge-case input#196
gaurav merged 17 commits into
masterfrom
fix-wrapper-edge-cases

gaurav commented Oct 14, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

hlapp left a comment

Uh oh!

Uh oh!

hlapp left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

gaurav commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The call worth overruling

What's fixed

Verified

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

hlapp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hlapp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gaurav commented Oct 14, 2026 •

edited

Loading