feat(profiles): a profile may name a per-suffix renderer
Arm E, capability only. A profile MAY name a renderer per suffix; no domain-aware renderer is written here, that stays a Non-Goal, and `_RENDERERS` is empty on purpose so the emptiness reads as a decision rather than an omission. THE LAYERING IS THE DESIGN, not an implementation detail. `extract.py` is the extraction registry and must not import the contract layer, or the dependency runs backwards and the registry stops standing on its own. So `extract_text` gains a keyword-only `renderer: Callable[[str], str] | None`, knowing nothing about profiles, and `inbox.py` -- which already holds the profile at that call site -- resolves a NAME to a function. A test asserts extract.py still contains no reference to the profile layer, because that constraint is the whole reason the parameter is shaped this way. The renderer runs AFTER extraction, never instead of it, so it never has to re-implement a reader and the two cannot drift. The default is identity, which is what keeps the five byte-pinned goldens byte-pinned -- asserted per suffix rather than once. An unknown renderer NAME is refused rather than falling back to identity: a silent fallback would produce a bundle that looks rendered and is not, which is the failure mode this arm exists to make visible. That needed a registered code (`unknown_renderer`) and its test -- slightly beyond the step's named files, but the capability cannot ship without defining what an unknown name does. `tests/test_profile.py`'s exact-field-set assertion went red, as the plan's risk table predicted. Updated deliberately with the reason recorded: that assertion exists so a field cannot arrive without someone deciding it should, and its red run is the mechanism working. Suite 917 -> 926. All five goldens byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
0170c526ad
commit
55a09d6c8e
7 changed files with 213 additions and 3 deletions
|
|
@ -141,6 +141,9 @@ class MaterializationError(IngestError):
|
|||
either of which would break frontmatter or an index link
|
||||
- `inbox_source_file_invalid` — an inbox `source_file` is multi-line and
|
||||
would inject frontmatter lines
|
||||
- `unknown_renderer` — a profile names a per-suffix renderer that is not
|
||||
registered; refused rather than falling back to identity, which would
|
||||
produce a bundle that looks rendered and is not
|
||||
- `okf_type_reserved` — an inbox concept claims the reserved 'verdict'
|
||||
layer (the same reservation ManifestError enforces at Door A)
|
||||
- `import_path_empty` — an external concept path reduces to an empty slug
|
||||
|
|
|
|||
|
|
@ -331,7 +331,9 @@ _OPTIONAL_EXTRACTORS: dict[str, Callable[[bytes], str]] = {
|
|||
}
|
||||
|
||||
|
||||
def extract_text(filename: str, data: bytes) -> str:
|
||||
def extract_text(
|
||||
filename: str, data: bytes, *, renderer: Callable[[str], str] | None = None
|
||||
) -> str:
|
||||
"""Convert one dropped file's bytes to OKF concept text, dispatched by type.
|
||||
|
||||
`filename` supplies the extension (case-insensitive); `data` is the raw
|
||||
|
|
@ -339,11 +341,24 @@ def extract_text(filename: str, data: bytes) -> str:
|
|||
without the extra, and any unregistered extension, fail fast with a typed
|
||||
:class:`ExtractionError`. Extracting a `pdf` also emits an
|
||||
:class:`ExtractionWarning`: drawn content has no text to recover.
|
||||
|
||||
`renderer`, when given, is applied to the EXTRACTED TEXT before it is
|
||||
returned -- after extraction, never instead of it, so a renderer never has
|
||||
to re-implement a reader and the two cannot drift. It is a plain callable
|
||||
rather than anything profile-shaped ON PURPOSE: this module is the
|
||||
extraction registry and must not import the contract layer, or the
|
||||
dependency would run backwards and the registry would stop standing on its
|
||||
own. Resolving a profile's NAMED renderer to a function is the caller's
|
||||
job, in the layer that already holds the profile.
|
||||
|
||||
The default is identity, which is what keeps every existing byte-pinned
|
||||
golden byte-pinned.
|
||||
"""
|
||||
suffix = Path(filename).suffix.lower()
|
||||
extractor = _CORE_EXTRACTORS.get(suffix) or _OPTIONAL_EXTRACTORS.get(suffix)
|
||||
if extractor is not None:
|
||||
return extractor(data)
|
||||
text = extractor(data)
|
||||
return renderer(text) if renderer is not None else text
|
||||
if suffix in _UNPARSED_OPTIONAL_EXTENSIONS:
|
||||
raise _extra_missing(suffix)
|
||||
raise ExtractionError(
|
||||
|
|
|
|||
|
|
@ -465,6 +465,44 @@ def _render_segments(
|
|||
return None
|
||||
|
||||
|
||||
# Arm E's resolution half. It lives HERE rather than in `extract.py` because
|
||||
# this is the layer that already holds the profile -- extraction takes a plain
|
||||
# callable and never learns what a profile is, which keeps the dependency
|
||||
# running from the contract layer down to the registry and not back up.
|
||||
#
|
||||
# A profile that names no renderers, or names none for this suffix, yields
|
||||
# `None`, and `extract_text`'s default is identity. That is what keeps the five
|
||||
# byte-pinned goldens byte-pinned while the capability exists.
|
||||
#
|
||||
# The registry is EMPTY on purpose: this step delivers the capability, not a
|
||||
# renderer. Writing a domain-aware renderer is a Non-Goal, and it is named as
|
||||
# unassigned here so the emptiness reads as a decision rather than an omission.
|
||||
_RENDERERS: dict[str, Callable[[str], str]] = {}
|
||||
|
||||
|
||||
def _resolve_renderer(profile: BundleProfile, filename: str) -> Callable[[str], str] | None:
|
||||
"""Map a profile's named renderer for this suffix to a function, or `None`.
|
||||
|
||||
An unknown NAME is an error rather than a silent fallback to identity: a
|
||||
profile naming a renderer that does not exist would otherwise produce a
|
||||
bundle that looks rendered and is not, which is the failure mode this whole
|
||||
arm exists to make visible.
|
||||
"""
|
||||
if profile.renderers is None:
|
||||
return None
|
||||
name = profile.renderers.get(Path(filename).suffix.lower())
|
||||
if name is None:
|
||||
return None
|
||||
try:
|
||||
return _RENDERERS[name]
|
||||
except KeyError as exc:
|
||||
raise MaterializationError(
|
||||
f"the profile names renderer {name!r}, which is not registered; "
|
||||
f"known renderers: {sorted(_RENDERERS)}",
|
||||
code="unknown_renderer",
|
||||
) from exc
|
||||
|
||||
|
||||
def process_inbox(
|
||||
inbox_dir: Path,
|
||||
bundle_dir: Path,
|
||||
|
|
@ -668,7 +706,9 @@ def process_inbox(
|
|||
continue
|
||||
outputs: list[tuple[str, str, tuple[str, ...]]] = []
|
||||
try:
|
||||
text = extract_text(path.name, source_bytes)
|
||||
text = extract_text(
|
||||
path.name, source_bytes, renderer=_resolve_renderer(profile, path.name)
|
||||
)
|
||||
covering = _plan_covering(segmentation, source_bytes)
|
||||
if covering is not None:
|
||||
blocked = _render_segments(
|
||||
|
|
|
|||
|
|
@ -908,6 +908,13 @@ class BundleProfile:
|
|||
# off" as a setting — it is the profile not having the capability at all,
|
||||
# which is what the downstream `is not None` checks read.
|
||||
segmentation: SegmentationPolicy | None = None
|
||||
# Arm E, capability only: a profile MAY name a renderer per suffix, applied
|
||||
# to extracted text before it becomes a concept body. `None` reads the same
|
||||
# way `segmentation` does -- the profile does not have the capability, not
|
||||
# "the capability is switched off". No domain-aware renderer exists in this
|
||||
# package; writing one is a Non-Goal and is named here as unassigned so the
|
||||
# absence is deliberate rather than an oversight.
|
||||
renderers: Mapping[str, str] | None = None
|
||||
|
||||
|
||||
# The ingest-spec + Phase 2 contract. Every value here was a constant in
|
||||
|
|
|
|||
|
|
@ -336,6 +336,18 @@ def test_unsupported_cell_type(value: object) -> None:
|
|||
assert code_of(excinfo) == "unsupported_cell_type"
|
||||
|
||||
|
||||
def test_unknown_renderer() -> None:
|
||||
from dataclasses import replace
|
||||
|
||||
from llm_ingestion_okf.inbox import _resolve_renderer
|
||||
from llm_ingestion_okf.profiles import DEFAULT
|
||||
|
||||
profile = replace(DEFAULT, renderers={".md": "no-such-renderer"})
|
||||
with pytest.raises(MaterializationError) as excinfo:
|
||||
_resolve_renderer(profile, "note.md")
|
||||
assert code_of(excinfo) == "unknown_renderer"
|
||||
|
||||
|
||||
# --- ExtractionError codes ---
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -227,4 +227,11 @@ def test_a_profile_is_assembled_from_its_policies() -> None:
|
|||
# exists to forbid. Defaulted to None, so the four shipped profiles
|
||||
# construct unchanged and their bytes do not move.
|
||||
"segmentation",
|
||||
# Arm E's capability, on the same terms. Updated DELIBERATELY: this
|
||||
# assertion pins the exact field set precisely so a field cannot arrive
|
||||
# without someone deciding it should, and the red run it produced is
|
||||
# the mechanism working rather than a regression. `None` means the
|
||||
# profile does not have the capability; every shipped profile still
|
||||
# constructs unchanged and no golden moved.
|
||||
"renderers",
|
||||
}
|
||||
|
|
|
|||
126
tests/test_render_hook.py
Normal file
126
tests/test_render_hook.py
Normal file
|
|
@ -0,0 +1,126 @@
|
|||
"""A profile may name a per-suffix renderer. Arm E, capability only.
|
||||
|
||||
No domain-aware renderer is written here -- that stays a Non-Goal, and naming
|
||||
it as unassigned is part of the point. What this pins is the seam: a profile
|
||||
CAN name one, the default is identity, and the layering direction does not move
|
||||
to make it possible.
|
||||
|
||||
The layering matters more than it looks. `extract.py` is the extraction
|
||||
registry and `profiles.py` is the contract layer; if extraction had to import
|
||||
profiles to find a renderer, the dependency would run backwards and the
|
||||
registry would stop being usable on its own. Instead `extract_text` takes a
|
||||
callable it knows nothing about, and `inbox.py` -- which already holds the
|
||||
profile -- is what resolves a name to a function.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import warnings
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from llm_ingestion_okf.extract import extract_text
|
||||
from llm_ingestion_okf.profiles import DEFAULT, SEGMENTED_V1, BundleProfile
|
||||
|
||||
FIXTURES = Path(__file__).parent / "fixtures"
|
||||
|
||||
|
||||
def _shout(text: str) -> str:
|
||||
"""A synthetic renderer: recognisable in output, dependent on nothing.
|
||||
|
||||
Modelled on the `_SYNTHETIC` instrument used elsewhere -- a real renderer
|
||||
would prove that a real renderer works; this proves the HOOK is called, and
|
||||
called with the extracted text.
|
||||
"""
|
||||
return f"<<{text}>>"
|
||||
|
||||
|
||||
def test_a_profile_may_carry_renderers_and_absence_is_none() -> None:
|
||||
"""`None` means the profile does not have the capability, never "off".
|
||||
|
||||
That is the established shape here: `segmentation` reads the same way, and
|
||||
every downstream check is `is not None`. A boolean flag would make
|
||||
"capability absent" and "capability present but disabled" the same value.
|
||||
"""
|
||||
assert DEFAULT.renderers is None
|
||||
assert SEGMENTED_V1.renderers is None
|
||||
assert "renderers" in {f for f in BundleProfile.__dataclass_fields__}
|
||||
|
||||
|
||||
def test_every_existing_profile_constructs_unchanged() -> None:
|
||||
"""The field is defaulted, so no existing constructor moved."""
|
||||
from llm_ingestion_okf import profiles as profiles_module
|
||||
|
||||
for name in dir(profiles_module):
|
||||
value = getattr(profiles_module, name)
|
||||
if isinstance(value, BundleProfile):
|
||||
assert value.renderers is None, f"{name} gained a renderer unasked"
|
||||
|
||||
|
||||
def test_extraction_without_a_renderer_is_byte_identical(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""The default is identity, asserted per suffix rather than once.
|
||||
|
||||
A hook whose default is not identity would move every golden in the repo,
|
||||
which is the failure this pins.
|
||||
"""
|
||||
cases = {
|
||||
"note.md": b"# Title\n\nBody\n",
|
||||
"note.txt": b"plain text\n",
|
||||
"data.json": b'{"a": 1}\n',
|
||||
"page.html": b"<p>hello</p>",
|
||||
}
|
||||
for name, payload in cases.items():
|
||||
assert extract_text(name, payload) == extract_text(name, payload, renderer=None)
|
||||
|
||||
|
||||
def test_a_named_renderer_is_what_reaches_the_caller() -> None:
|
||||
text = extract_text("note.md", b"# Title\n\nBody\n", renderer=_shout)
|
||||
assert text == "<<# Title\n\nBody\n>>"
|
||||
|
||||
|
||||
def test_the_renderer_runs_after_extraction_not_instead_of_it() -> None:
|
||||
"""It receives EXTRACTED text, not raw bytes.
|
||||
|
||||
A renderer handed the raw bytes would have to re-implement extraction, and
|
||||
the two would drift. The csv case makes the ordering visible: the renderer
|
||||
sees the rendered markdown table, not the comma-separated source.
|
||||
"""
|
||||
seen: list[str] = []
|
||||
|
||||
def capture(text: str) -> str:
|
||||
seen.append(text)
|
||||
return text
|
||||
|
||||
extract_text("rows.csv", b"a,b\n1,2\n", renderer=capture)
|
||||
assert seen and "|" in seen[0], "the renderer did not receive extracted text"
|
||||
assert "a,b" not in seen[0].split("\n")[0] or "|" in seen[0]
|
||||
|
||||
|
||||
def test_a_renderer_applies_to_binary_types_too() -> None:
|
||||
data = (FIXTURES / "two-line-krav.docx").read_bytes()
|
||||
with warnings.catch_warnings():
|
||||
warnings.simplefilter("ignore")
|
||||
text = extract_text("krav.docx", data, renderer=_shout)
|
||||
assert text.startswith("<<") and text.endswith(">>")
|
||||
|
||||
|
||||
def test_the_extraction_registry_does_not_import_the_profile_layer() -> None:
|
||||
"""The layering direction, asserted rather than intended.
|
||||
|
||||
This is the constraint that shaped the design: the hook is a callable
|
||||
parameter precisely so extraction never needs to know what a profile is.
|
||||
"""
|
||||
source = (
|
||||
Path(__file__).resolve().parents[1] / "src" / "llm_ingestion_okf" / "extract.py"
|
||||
).read_text(encoding="utf-8")
|
||||
assert "profiles" not in source, "extract.py must not reach into the profile layer"
|
||||
|
||||
|
||||
def test_renderer_is_keyword_only() -> None:
|
||||
"""Positional call sites stay source-compatible, which is the repo's rule
|
||||
for every new public parameter."""
|
||||
with pytest.raises(TypeError):
|
||||
extract_text("note.md", b"x", _shout) # type: ignore[misc]
|
||||
Loading…
Add table
Add a link
Reference in a new issue