refactor(intrinsics): resolve the adapter's output contract from the adapter, not a parallel argument - #1556
Draft
planetf1 wants to merge 5 commits into
Draft
refactor(intrinsics): resolve the adapter's output contract from the adapter, not a parallel argument#1556planetf1 wants to merge 5 commits into
planetf1 wants to merge 5 commits into
Conversation
…gistry Introduces mellea/backends/adapters/io_contracts.py: a capability-keyed registry of IOContract instances, keyed by the same catalog name passed to call_intrinsic() and resolve_adapter(). Moves _ListContract (from rag.py) next to the existing _DictContract in _core.py, adds the guardian-specific contracts, and adds _RequirementCheckContract to consolidate the score-range validation core.requirement_check() previously hand-rolled after each call. This is the single source of truth a later commit wires resolve_adapter() and the intrinsic helpers to consume, instead of each declaring its own IOContract instance that could silently drift from the other's. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
…ot a parallel argument call_intrinsic() resolved the adapter and discarded it, taking the output contract instead as a separate io_contract= argument that each caller supplied from its own module-level Adapter constant. Nothing tied the two together, so a caller could pass a contract that didn't match the adapter resolve_adapter() actually returned. call_intrinsic() now keeps the adapter resolve_adapter() returns and calls its own io_contract.parse() on the raw output; the io_contract= parameter is gone, so a mismatched pair is no longer expressible. IntrinsicAdapter and EmbeddedIntrinsicAdapter (the shims resolve_adapter() constructs) now look up their contract in the io_contracts registry instead of the _ShimIOContract placeholder, which is now unreachable and removed. The ten module-level Adapter constants in rag.py/guardian.py keep their identity and weights as before, and read io_contract from that same registry rather than declaring their own instance. core.py's three helpers (check_certainty, requirement_check, find_context_attributions) previously had no declared contract at all — the first two skipped validation (raw json.loads), and requirement_check hand-rolled its own score-range check after the call. All three now have a declared contract in the registry; requirement_check's hand-rolled validation is replaced by _RequirementCheckContract, and find_context_attributions reads its now-list-wrapped result via ["items"]. Weights binding is untouched — this stays entirely on the io_contract axis (the design discussion in generative-computing#1486 that split the two). The ten constants' placeholder LocalFileBinding() and the backed-out Adapter.__post_init__ cross-check remain future work, as noted in _core.py. Verified against real granite-4.1-3b weights: test/stdlib/components/ intrinsic/test_core.py, test_rag.py, and test_guardian.py (qualitative, GPU-gated) all pass on this run — 3 passed + 2 pre-existing xfails (non-deterministic attribution count, tracked separately) for core.py, 14/14 for rag.py, 6/6 for guardian.py. No test constructs a resolve_adapter() result carrying the old placeholder contract; test_io_contracts.py's registry-completeness test guards that going forward. Fixes generative-computing#1516 Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
…d constants, fix real duplicate
Three independent reviewers found the same gap from different angles: the
registry this PR introduces to close the parallel-declaration problem still
had one. mellea/stdlib/requirements/requirement.py's requirement_check_to_bool()
hand-rolled the exact score-range validation just consolidated into
_RequirementCheckContract, with a comment pointing at code this PR deleted
from core.py. It now delegates to get_io_contract("requirement-check").parse(),
which is a strict improvement on its undocumented AttributeError-on-non-dict
failure mode (now the documented ValueError the contract raises).
Also, per review:
- Export get_io_contract from mellea.backends.adapters.__init__ (and __all__),
matching the sibling adapter-package imports. Unexported, it was invisible to
the docs pipeline (io_contracts.mdx was pruned as "not imported by
__init__.py") and to the AGENTS.md-mandated docstring quality gate, despite
being the function the module's own docstring designates as the mandatory
entry point.
- Delete _UNCERTAINTY_ADAPTER and _CONTEXT_ATTRIBUTION_ADAPTER from core.py:
nothing referenced them — their only purpose (supplying io_contract= to
call_intrinsic) was exactly what the prior commit removed. Keep
_REQUIREMENT_CHECK_ADAPTER, which test_core_schema.py uses as a
resolve_adapter() stub.
- Add a regression test per shim class (test_shims.py) asserting
IntrinsicAdapter/EmbeddedIntrinsicAdapter carry the real registry contract,
not a placeholder. Without it, reverting get_io_contract(intrinsic_name)
back to a stub would have passed every existing test in the file. Verified
by temporarily reintroducing a stub object in place of get_io_contract(): both
new tests failed as expected, then passed again once reverted.
- Enforce the registry's exhaustiveness over known_intrinsic_names() at import
time in io_contracts.py, mirroring the existing duplicate-effective_capability
check in catalog.py, rather than relying solely on a test.
- Correct test_core_schema.py's module docstring, which claimed resolve_adapter
itself runs; only its stubbed return value does.
Assisted-by: Claude Code
Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Follow-up to the review response commit — the lower-severity items all three reviewers raised, applied where they were cheap and genuinely useful: - adapter.py: convert the remaining `.. deprecated::` RST directives on IntrinsicAdapter/EmbeddedIntrinsicAdapter to Google-style `Deprecated:` sections, matching the `Note:` conversion already done on the same docstrings. (CustomIntrinsicAdapter's directive is untouched by this diff and left alone.) - _core.py / io_contracts.py: replace the stale "not used in Phase 1; implemented in Phase 2" build_prompt placeholder message — self-contradictory now that this module *is* the Phase 2 work — with an accurate description of the current state. Updated the one test asserting on the old wording. - _core.py: module docstring now names _ListContract and explains the generic-vs-capability-specific split with io_contracts.py. - io_contracts.py: get_io_contract's docstring now states its keys are catalog `name`s, not `effective_capability` tokens, and corrects "permissive" to mean permissive about which keys are present, not about the JSON shape. Added the matching inline comment on the fallback return. - io_contracts.py: comment distinguishing the two AdapterSchemaMismatchError raise sites in _PolicyGuardrailsContract (neither key present vs. both). _RequirementCheckContract's docstring now names both production consumers it consolidates (core.py and requirement.py) with full paths. - test_io_contracts.py: new non-GPU test feeding the recorded context-attribution model output (test/stdlib/components/intrinsic/testdata) through the real contract. The GPU-gated equivalent is xfail(strict=False) for unrelated non-determinism, so it gives no CI signal on schema drift; this closes that gap without a GPU. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
Applies the multi-party review findings on this PR (issue generative-computing#1516): - test_core_contracts.py: CI-runnable wiring tests for check_certainty and find_context_attributions — the two helpers that moved to registry-contract validation (plus the items unwrap) with only GPU-gated qualitative/xfail coverage before - test_requirement.py: cover the newly-typed ValueError for non-object JSON in requirement_check_to_bool (was an undocumented AttributeError) - test_io_contracts.py: guard the reverse direction of the registry exhaustiveness invariant (orphan keys) - core.py: the new Raises: ValueError entries were narrower than the contracts' actual raise paths (wrong top-level shape is also ValueError) - io_contracts.py: the string literal after the registry assignment was a dead expression — dicts have no docstring; make it a comment - test_rag_contracts.py / test_guardian_io_contract.py: the contracts no longer live in rag.py/guardian.py (Phase 1 -> io_contracts.py, generative-computing#1516) - rag.py / guardian.py / core.py: state consistently that the per-helper Adapter constants are weights scaffolding for generative-computing#1141/generative-computing#1142, not a second contract source Assisted-by: opencode Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem, in plain terms
Mellea's adapter functions (LoRA/aLoRA capabilities like answerability
checking, hallucination detection, guardrails) each produce output in their
own JSON shape, and something has to check that shape before the caller trusts
it. Before this PR, which adapter actually ran and what shape its output
should be were decided in two unconnected places: the backend resolved the
adapter, then the high-level helper (
check_answerability,guardian_check,...) separately supplied a validator built from its own module-level constant.
Nothing enforced that the two matched. Passing the wrong validator for the
adapter that actually ran was possible and silent — a caller could think it
was checking one capability's output shape while actually resolving another
capability's adapter, or vice versa.
The impact was concentrated on
core.py's three helpers(
check_certainty,requirement_check,find_context_attributions):check_certaintyandfind_context_attributionshad no validation at all(raw
json.loads, so a malformed or schema-drifted response failed with abare
KeyError/IndexErrorfar from the cause), andrequirement_checkcarried its own hand-rolled copy of the score-range check that a second,
independent copy in
mellea/stdlib/requirements/requirement.pyalso carried— two places that had to be kept in sync by hand and weren't.
Where this sits in Epic #929
Epic #929 (Adapter Function Lifecycle) restructures how Mellea composes
adapters — an
AdapterisIdentity+IOContract+WeightsBinding,where
IOContractvaries by capability andWeightsBindingvaries bydeployment. Discussion #1486 agreed that split explicitly; #1516 is Phase 2's
work on the
IOContract(capability) axis only. TheWeightsBinding(deployment) axis is deliberately untouched here — activation/
apply_activationcode is not part of this PR, by design, per that discussion.
This PR blocks #1144 (Phase 4 — removing the shim adapter classes
IntrinsicAdapter/EmbeddedIntrinsicAdapterentirely), since that removalneeds the shims to already carry real contracts rather than the Phase 1
_ShimIOContractplaceholder. It also blocks #1358. Fixes #1516 itself.What changed
Commits 1–2: the core refactor
mellea/backends/adapters/io_contracts.py(new): a single,capability-keyed registry of
IOContractinstances, keyed by the samecatalog name passed to both
call_intrinsic()andresolve_adapter(), andexported as
mellea.backends.adapters.get_io_contract. Its exhaustivenessover the adapter-function catalog is enforced at import time (mirroring the
existing duplicate-
effective_capabilitycheck incatalog.py).call_intrinsic()no longer takes anio_contract=argument. It calls.io_contract.parse()on the adapterresolve_adapter()actually returns— the validator now travels with the adapter, so a mismatch between the two
is no longer expressible.
IntrinsicAdapter/EmbeddedIntrinsicAdapter(the shimsresolve_adapter()constructs) now look up their contract in the registryinstead of the
_ShimIOContractplaceholder, which is now unreachableeverywhere and deleted.
Adapterconstants inrag.py/guardian.pykeep their
identityandweightsas before, but now readio_contractfrom the same registry instead of declaring their own instance.
core.py's three helpers each gained a declared contract for the firsttime.
requirement_check's hand-rolled score-range validation moved into_RequirementCheckContract;find_context_attributionsreads itsnow-list-wrapped result via
["items"].Commits 3–4: independent code review, then fixes
Before merging, I ran an independent 3-reviewer panel (exhaustive / pragmatic
/ pattern-matching perspectives) against the diff. Two reviewers
independently found, from different angles, that the PR's own "single source
of truth" claim was false for the one capability it had done the most work
on:
requirement_check_to_bool()(mellea/stdlib/requirements/requirement.py)— the other production consumer of
requirement-checkoutput, used byALoraRequirement— still hand-rolled the exact validation this PR had justconsolidated elsewhere, with a comment pointing at code the PR had deleted.
Fixed: it now delegates to
get_io_contract("requirement-check").parse(...).Also fixed from review:
get_io_contractwasn't exported frommellea.backends.adapters.__init__,so the docs pipeline pruned its page and the AGENTS.md-mandated docstring
quality gate never saw it, despite being the function the module's own
docstring designates as the mandatory entry point. Now exported.
Adapterconstants incore.py(
_UNCERTAINTY_ADAPTER,_CONTEXT_ATTRIBUTION_ADAPTER) were dead code —nothing referenced them once
io_contract=was removed fromcall_intrinsiccall sites. Deleted; kept
_REQUIREMENT_CHECK_ADAPTER, which a test uses asa
resolve_adapter()stub.contract rather than a placeholder — every other test would still pass if
that wiring were reverted. Added one regression test per shim class;
verified each fails against a reintroduced placeholder and passes against
the real fix.
stale RST directives left over from an earlier docstring pass, a
self-contradictory "not used in Phase 1; implemented in Phase 2" placeholder
message inside the Phase-2 module itself, ambiguous file references in
docstrings, a fallback-contract docstring that undersold what it actually
guards against, and a non-GPU unit test exercising the recorded
context-attributionmodel output against the real contract (its GPU-gatedequivalent is
xfail(strict=False)for unrelated non-determinism, so itgave no CI signal on schema drift).
Behaviour changes worth flagging
Both are validation additions, not regressions, but they change what a
caller catches:
check_certaintypreviously returned a plainKeyErrorfor a responsemissing
certainty; it now raisesAdapterSchemaMismatchError.requirement_check_to_bool()previously raised an undocumentedAttributeErrorfor non-object JSON input (e.g. a bare JSON array); it nowraises the documented
ValueErrorits contract'sparse()raises for thesame input.
Deliberately out of scope
Per the #1486 capability/deployment split: the ten pre-existing constants'
placeholder
LocalFileBinding()weights, and theAdapter.__post_init__identity/weights cross-check that was tried and backed out during #1454 —
both are deployment-axis concerns, noted as future work in
_core.py.Activation/
apply_activationcode is untouched throughout.Verification against real weights
Ran the qualitative, GPU-gated suites against
ibm-granite/granite-4.1-3b(Apple Silicon MPS, cached weights) rather than relying on mocks alone —
covering all 13 catalogued adapter functions' contracts against real model
output:
test/stdlib/components/intrinsic/test_core.py— 3 passed, 2 xfailed(pre-existing, tracked non-determinism in attribution count — not a schema
failure; confirmed with
--runxfailthat the failure is a plainlist-equality
AssertionError, notAdapterSchemaMismatchError).test/stdlib/components/intrinsic/test_rag.py— 14/14 passed.test/stdlib/components/intrinsic/test_guardian.py— 6/6 passed.Test plan
test/backends/test_adapters/test_io_contracts.py(new) — registrycompleteness against
known_intrinsic_names(), the permissive-about-keysfallback for names outside the catalog (e.g.
CustomIntrinsicAdapter),_RequirementCheckContract's score-range validation, and the recordedcontext-attributionfixture parsed without a GPU.test/backends/test_adapters/test_shims.py— new:IntrinsicAdapterand
EmbeddedIntrinsicAdaptercarry the real registry contract, not aplaceholder (verified to fail on a reintroduced placeholder, pass on the fix).
test_rag_contracts.py/test_guardian_io_contract.py—unchanged, still exercise the same contract objects (now sourced from the
registry) directly.
test_core_schema.py— rewritten to exercise the realcall_intrinsic→IOContract.parsepath against a stubbedresolve_adapterreturn value, since the validation it tests no longerlives in
core.py.test_util_unit.py— new:call_intrinsicparses via the resolvedadapter's
io_contract, and no longer accepts anio_contract=kwarg.test_requirement.py(requirement_check_to_bool) — everypre-existing case passes unchanged against the delegated implementation.
uv run pytest -m "not qualitative"— full suite, 3800+ passed / 5failed (all in
test_python_tool.py, pre-existing and unrelated — Dockerisn't available in this environment).
ruff format --check .,ruff check .,uv run mypy .— all clean.(
tooling/docs-autogen/audit_coverage.py --quality --fail-on-quality --threshold 100)— 100% coverage, 0 issues.
Fixes #1516