feat(tavi): rebuild definition networks from cubes on read

[jfrench9]

Summary

A TAVI-loaded filing now has its definition linkbase back. The TAVI emitter writes dimensional wiring as cube objects, and the reader skipped them, so information_block found no hypercubes on a TAVI load: on JPM's FY2025 10-K, 0 against the holon's 140.

The TAVI-first published load and the INF-precision fix were pushed here after this merged and are not in it; they are #72.

Changes

• deserialize/tavi.py: _definition_networks walks each cube back into all, hypercube-dimension, dimension-domain, domain-member and dimension-default arcs, one definition network per role (cubes join roles through groupContents). _networks' group-to-role mapping is factored into _roles.
• A cube records no hypercube element, primary items or default member, so the reader takes the table from the presentation parent the cube's axes share in their own role (a table no role presents, or already claimed in the role, keeps the cube's name rather than a guess), the primary items from that table's other children, and an optional axis's default from its domain. A recognised table is marked is_hypercube_item.
• tests/test_deserialize.py: the test that asserted TAVI loses the definition networks now asserts it rebuilds them (following a targetRole hop); new tests cover a presented table and an optional axis's default.

Output Impact

INTERNAL for every emitted file, broader coverage on read.

Filing	Hypercubes before	After	Holon
JPM 10-K	0	136	140
BAC 10-K	0	112	116

Every axis, domain and member set in the rebuilt cubes is identical to the holon's. The remainder is what a cube cannot carry (tables presented under another element, axis-less tables, non-domain defaults), raised as a comment on the TAVI draft.

Testing

• just test-all: 547 passed, 2 skipped; ruff, format and basedpyright clean.

[jfrench9]

@claude please review this PR

[claude]

Claude finished @jfrench9's task in 1m 46s —— View job

---

Reviewing PR #71

• Gather PR metadata, diff, and existing review feedback
• Read repo CLAUDE.md for conventions (none present in this repo)
• Review xbrlkit/deserialize/tavi.py changes
• Review tests/test_deserialize.py changes
• Post review findings

PR Summary

Title: feat(tavi): rebuild definition networks from cubes on read
Author: jfrench9 | Branch: feature/tavi-definition-networks → main
Changes: +264 / −28 across 2 files

Adds _definition_networks to xbrlkit/deserialize/tavi.py, walking each emitted cube back into all / hypercube-dimension / dimension-domain / domain-member / dimension-default arcs, one definition network per role. Factors the group→role mapping out of _networks into a shared _roles helper. Infers the three things a cube doesn't record (hypercube element, primary items, optional-axis default) from the presentation tree, marking a recognized table is_hypercube_item.

Existing Review Feedback

No prior human or bot reviews, and no inline comments on this PR — this is the first pass.

My Review

I read the diff against origin/main in full (_definition_networks, the refactored _roles, _table_of, and both new/changed tests) and traced the arc construction against the emitter side (xbrlkit/serialize/tavi.py) and the consumer (information_block.build_hypercubes). I was not able to execute just test-all or pytest in this environment (Bash commands required interactive approval that wasn't available), so this is a static review — the PR body's own parity numbers (JPM/BAC against the holon) are the best evidence the logic is correct end-to-end.

Issues (should fix before merge)

None found — I didn't find a correctness bug in the arc-building logic. The domain-member flattening correctly relies on the emitter having already resolved targetRole hops into domainNetworks[*].relationships (confirmed in serialize/tavi.py:711-717), so the reader doesn't need to re-walk roles itself. Typed axes (no domainNetwork) are correctly skipped before the optional-default logic runs (tavi.py ~line 660), so a typed+optional axis doesn't get a spurious dimension-default arc.

Suggestions (non-blocking improvements)

1. _table_of tie-break is silent. When two presentation parents share every axis of a cube (ambiguous table), sorted(common)[0] picks the alphabetically-first one with no signal that the choice was ambiguous (tavi.py in _table_of). Given the PR's own numbers this is evidently rare in practice (0 discrepancies against the holon on the two sampled filings), so not blocking — but a code comment noting why alphabetical order was chosen as the tiebreak (vs., say, "first encountered") would help the next reader trust it's deliberate rather than incidental.
2. Self-referential fallback arc. When a cube's role presents no table at all, hypercube falls back to the cube's own synthetic name (e.g. rpt:cube-3), and since that name has no presentation children, primary_items also falls back to [hypercube] — producing an all arc with from_qname == to_qname. This is clearly intentional (matches the doc comment "the cube's own name stands in") and is exercised by test_tavi_rebuilds_the_definition_network_from_its_cube, but worth double-checking that information_block.build_hypercubes (and any other arc consumer) doesn't choke on a hypercube whose sole primary item is itself — looked fine on read, but it's a slightly unusual shape.

Questions

None — the PR description's inline comments on the three "not properties of our own" omissions (ecd roles, axis-less tables, non-domain defaults) already anticipate the natural follow-up questions.

Verdict

APPROVE. The change is well-scoped (read-side only, no emitted-file impact as stated), the docstrings clearly justify each inference the reader makes, and the new tests cover the three new code paths (table presented, table not presented/name-collision fallback, optional-axis default). The two suggestions above are polish, not blockers.

Note: I could not run just test-all / pytest / ruff directly in this session (Bash execution required approval that wasn't granted), so I'm relying on static reading plus the PR author's reported 547 passed, 2 skipped and clean lint/type-check — worth a second confirmation from CI before merge if it hasn't run yet.

[jfrench9]

@claude please review this PR — it now also carries the TAVI-first published load and the INF-precision fix (two new commits).

[claude]

Claude finished @jfrench9's task in 2m 57s —— View job

---

Reviewing PR #71 (requested: review of the two new commits)

• Gather PR metadata, diff, and existing review feedback
• Review TAVI-first published load (serve/session.py)
• Review INF-precision fix (deserialize/tavi.py)
• Review test coverage for new commits
• Post review findings

Finding: the two new commits aren't in this checkout

I can't find either of the changes described in your comment in the repository as checked out for this run (main @ 15a88d6, the same commit git status showed at session start). Concretely:

• TAVI-first published load — xbrlkit/serve/session.py still has the pre-PR shape: PublishedFiling.holon_url is required (not optional), there's no tavi_url field, no model_urls, and _published_from only recognizes rep.get("kind") of "holon" or "document" — no "tavi" case. A repo-wide grep for tavi_url and model_urls returns zero matches anywhere in xbrlkit/.
• INF-precision fix — deserialize/tavi.py:831,845 still reads an absent decimals key as None:

  decimals = value_obj.get("decimals")
  ...
  decimals=None if decimals is None else str(decimals),

  That's the bug your PR description says commit 2 fixes, not the fix — an absent decimals should become "INF" for a numeric fact, since serve/tools.py:_precision() maps None to -inf (least precise) and "INF" to +inf (most precise), which is exactly the "exact fact lost to its own rounded duplicate" failure mode the PR body describes.
• Abstract domains/members — the module docstring at deserialize/tavi.py:22-31 still lists "the abstractness of axes, domains and members" as one of the four things this reader does not reconstruct, and _bare()'s calls for domainClasses/members/dimension domains still omit is_abstract=True.
• tests/test_deserialize.py has no INF literal anywhere, and tests/test_tavi.py's only INF references are the pre-existing serializer tests (that omitting decimals on write is correct) — there's no new deserialize-side test asserting a round-tripped fact reads back with decimals == "INF".

The merge commit message (15a88d60) is also still feat(tavi): rebuild definition networks from cubes on read — the single-feature title from before your comment — not the three-feature title now on the PR. Everything here is consistent with: the checkout for this job predates the two new commits.

I also can't independently confirm via gh pr view — network/git fetch calls require interactive approval that isn't available in this run (same limitation as the first review).

Could you confirm the two commits were actually pushed to feature/tavi-definition-networks? If they're on the branch but this checkout is stale, re-triggering @claude should pick up a fresh clone. If you pushed them somewhere else (a different branch, or after this PR's merge), point me at it and I'll review that instead.

For reference, my review of the definition-networks rebuild (the code that is present here) stands from the earlier pass: no correctness issues found, two non-blocking suggestions (_table_of's silent alphabetical tie-break, and the self-referential fallback all arc when a cube's role presents no table) — see the previous comment for detail.

[jfrench9]

Correction: the last two commits reached this branch after it merged, so the TAVI-first published load and the INF-precision fix are not in this merge. They are in #72. The description above now matches what merged.