Skip to content

feat: preserve Context subtype and enforce input==output context type - #1582

Merged
AngeloDanducci merged 8 commits into
generative-computing:mainfrom
AngeloDanducci:ad-1522
Sep 11, 2026
Merged

AngeloDanducci merged 8 commits into
generative-computing:mainfrom
AngeloDanducci:ad-1522

Conversation

@AngeloDanducci

Copy link
Copy Markdown
Contributor

Pull Request

Issue

Fixes #1522

Description

preserve Context subtype and enforce input==output context type

Testing

  • Tests added to the respective file if code was changed
  • New code has 100% coverage if code was added
  • Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

Attribution

  • AI coding assistants used

Adding a new component, requirement, sampling strategy, or tool?

If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.

  • Component
  • Requirement
  • Sampling Strategy
  • Tool

NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.

Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
@AngeloDanducci
AngeloDanducci requested a review from a team as a code owner August 25, 2026 15:31
@github-actions github-actions Bot added the enhancement New feature or request label Aug 25, 2026

@jakelorocco jakelorocco left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good; I have a few concerns / thoughts that I think would be worth addressing in this PR so that we can get a better surface for the context typing.

Comment thread mellea/stdlib/functional.py Outdated
Comment thread test/stdlib/test_context_type_enforcement.py
Comment thread mellea/stdlib/functional.py
…verride

Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Comment thread mellea/stdlib/functional.py Outdated
Comment on lines +834 to +839
for sample_ctx in sampling_result.sample_contexts:
_enforce_context_type(
context,
sample_ctx,
allow_context_type_change=allow_context_type_change,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add new tests for this as well?

Comment thread mellea/core/base.py
)


class Context(abc.ABC):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the implementation for our existing contexts and I think they will require fixes here as well. It looks like several of their methods use the named class constructor instead of self, etc... which will cause subclasses to fail.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked the constructor paths at b4ca7bce. Using type(self) fixes bare-subclass demotion, but subclasses with required constructor arguments now fail: ChatContext._make_root() calls type(self)(), while both built-in add() paths reach Context.from_previous() → cls(). A subclass with __init__(self, tag: str) raises TypeError on its first add(); ChatContext also fails through new_instance() during reset/model binding.

Please either preserve subtype state without re-running subclass initialisers, including the Context.from_previous() path, or explicitly require subclasses to be constructible with no arguments. Add a required-argument-subclass regression test for the chosen contract.

Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>

@planetf1 planetf1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a few API, typing, and lifecycle issues inline. The new runtime subtype coverage and the checks over every sampling context are both useful additions.

Comment thread mellea/stdlib/functional.py Outdated
return sampling_result
else:
return result, new_ctx
checked_ctx = _enforce_context_type(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One lifecycle ordering issue: the context guard runs after component_post_success. When it rejects new_ctx, the exception handler then emits component_post_error, so plugins see both terminal events. I reproduced ['success', 'ContextTypeMismatchError']; the tracing plugin closes the action span as successful before the error hook runs. Could we validate the returned context(s) before constructing the success payload, so a rejected call emits only the error path?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, done.

ContextTypeMismatchError: If the output context type differs from the
input context type and `allow_context_type_change` is `False`.
"""
if type(output_ctx) is type(input_ctx) or allow_context_type_change:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This escape hatch is runtime-correct but statically unsound. With allow_context_type_change=True, a ChatContext can become a SimpleContext, yet this cast and all public overloads still return ContextT. That also leaves MelleaSession[ChatContext].ctx typed as ChatContext after the switch, so session.ctx.model_id type-checks even though the runtime object has no such attribute. Could the Literal[True] overloads widen to Context, with sessions that permit switching represented as MelleaSession[Context]?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Widened this, could use another set of eyes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked at b4ca7bce. The literal-True widening is in place for the existing functions, but three gaps remain:

  1. achat has no overloads, so allow_context_type_change=True still returns tuple[Message, ChatContext] rather than widening the context. Please mirror chat and add a typing assertion.
  2. MelleaSession(..., allow_context_type_change=True) remains MelleaSession[ChatContext], unlike start_session, which widens to MelleaSession[Context].
  3. A runtime bool matches neither Literal overload. This leaves nine suppressed internal calls inferred as Any, and makes start_session(ctx=..., allow_context_type_change=flag) a public call-overload error. Add bool → Context fallback overloads for each applicable non-SamplingResult return shape and the ctx-supplied start_session forms.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two cases remain here, with different practical weight.

Required for this PR: the context_type and default-context overloads accept any bool but always return a narrowly typed session:

session = start_session(
    context_type="chat",
    allow_context_type_change=True,
)
reveal_type(session.ctx)  # ChatContext

The session is allowed to switch to another context subtype, so this should be MelleaSession[Context]. This is a normal public construction path and directly within the PR's typing scope. Could these overloads also distinguish Literal[False], Literal[True], and a runtime bool, with matching assert_type checks?

A separate low-likelihood, non-blocking edge is that allow_context_type_change remains writable after construction. Mypy accepts changing it to True on an existing MelleaSession[ChatContext], after which the runtime context can change subtype while session.ctx remains statically ChatContext. Nothing in-tree does this, but making the setting read-only would close that gap.

Comment thread mellea/core/base.py Outdated

@abc.abstractmethod
def add(self, c: Span) -> Context:
def add(self, c: Span) -> Self:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this public abstract method from Context to Self is a source-compatible runtime change, but it breaks typed third-party subclasses: an existing def add(...) -> Context now fails mypy's override check. Is that breaking API change intentional? If not, it may be safer to retain the abstract Context return and provide the narrower Self types on the concrete built-in contexts.

@AngeloDanducci AngeloDanducci Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should now narrow to self, the return while the abstract
signature stays Context for source-compat

Comment thread mellea/stdlib/context/chat.py Outdated
node._model_id = model_id

ctx: ChatContext = ChatContext.__new__(ChatContext)
ctx: ChatContext = cls.__new__(cls)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preserving the subclass here bypasses its initializer and restores only the three built-in fields. That conflicts with _propagated_fields, whose class documentation invites subclasses to add state there. I reproduced a subclass-owned field disappearing after window compaction, then raising AttributeError on the next add(); a subclass with a required constructor argument already fails on the first add() through type(self)(). This needs a subclass-aware factory or clone/rebuild hook rather than cls.__new__().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should now properly propagate

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rebuild now preserves the concrete subtype, but the new compaction test only checks type identity. The original regression was a subclass-owned _propagated_fields value being discarded. Please extend it with a ChatContext subclass that registers an extra field, trigger compaction, and assert that field survives on the rebuilt context.

Comment thread mellea/stdlib/context/chat.py Outdated
new = ChatContext.from_previous(self, c)
# `type(self)`, not `ChatContext`, so a subclass gets an instance of
# itself back rather than being silently demoted to `ChatContext`.
new = type(self).from_previous(self, c)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new runtime subclass checks are helpful. There is still a static half to the same contract: add() is annotated as returning ChatContext, and SimpleContext.add() returns SimpleContext. Mypy therefore widens a bare subclass at its first add(), despite the runtime object retaining its subtype. Could these use Self (and the compactor path preserve that type), with subclass assert_type checks added alongside the runtime tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ChatContext.add and SimpleContext.add now return Self, and the
compactor path preserves it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The built-in compactors preserve the concrete context type, but the InlineCompactor extension contract still permits a custom compactor to return a plain ChatContext:

class DemotingCompactor(InlineCompactor):
    def compact(self, ctx: ChatContext, *, backend=None) -> ChatContext:
        return ChatContext()

result = CustomContext(compactor=DemotingCompactor()).add(message)
reveal_type(result)  # CustomContext
print(type(result))  # ChatContext

I reproduced both results. This requires the less common combination of a custom ChatContext subtype and custom inline compactor, but both are supported extension points and add() now promises Self, so I think it is within this PR's scope. Could the compactor contract preserve its input subtype, or could add() validate the returned type before casting? A custom-compactor regression test would pin the guarantee.

Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Comment thread mellea/core/base.py Outdated
return cls()

def new_instance(self) -> Context:
def new_instance(self) -> Self:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate source-compatibility issue on the sibling factory: Context.new_instance() now returns Self. A third-party override returning Context—the previous base contract, and one the docstring invites—now fails mypy’s override check.

Please keep the base return type as Context, matching add(). ChatContext.new_instance() can narrow its own return to Self, preserving subtype inference without breaking existing overrides.

await_result: bool = False,
) -> tuple[ModelOutputThunk[S], Context] | SamplingResult:
) -> tuple[ModelOutputThunk[S], ContextT] | SamplingResult:
"""Asynchronous version of .act; runs a generic action, and adds both the action and the result to the context.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document the sampling-context validation scope. With return_sampling_results=False, only the chosen result_ctx is returned and checked; with True, every sample_contexts entry is returned and validated. That distinction is coherent, but it is surprising enough to deserve one sentence in the public API documentation.

def _rebuild_chat_context(
components: list[Span],
*,
source: ChatContext,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this compatibility break intentional? The documented custom-compactor recipe imports this underscore-private helper directly, so copied versions of the old call shape now fail because source is required; None for the configuration arguments also now inherits from source instead of clearing the field.

If intentional, please note the migration. If not, preserve the previous call shape with a source=None fallback.

@planetf1 planetf1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at b4ca7bce: the previous fixes hold. Requesting changes for the five remaining issues called out inline: the incomplete literal/runtime-bool overload contract, required-argument context subclasses, and new_instance() source compatibility. The sampling-scope and compactor-compatibility comments are non-blocking.

Required CI is green at this head.

Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>

@planetf1 planetf1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at a2f8a0e4: the five previously requested items are addressed. Requesting changes for two remaining in-scope type-safety gaps: start_session does not widen sessions created through context_type or the default context when switching is enabled, and a custom InlineCompactor can invalidate ChatContext.add() -> Self. The mutable session-flag case is low-likelihood and non-blocking; details inline.

Verified with full-repository mypy, Ruff, targeted context tests, and focused mypy/runtime reproducers.

…emotion

Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>

@planetf1 planetf1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@AngeloDanducci
AngeloDanducci added this pull request to the merge queue Sep 11, 2026
Merged via the queue into generative-computing:main with commit 8bcb2b9 Sep 11, 2026
11 checks passed
@AngeloDanducci
AngeloDanducci deleted the ad-1522 branch September 11, 2026 17:19
noaakl added a commit to noaakl/mellea that referenced this pull request Sep 14, 2026
…nit__

`with_sent_token_ids` built its copy with `type(self)()`. The rebase onto `main` brought
generative-computing#1582, whose point is that a `ChatContext` subclass may take required constructor
arguments: `add()` and `_make_root()` therefore build nodes with `__new__` and copy
`_propagated_fields` rather than calling the initializer. This method was the one
remaining node factory still calling it, so a subclass like

    class Tagged(ChatContext):
        def __init__(self, tag: str, **kw): ...

raised `TypeError: Tagged.__init__() missing 1 required positional argument: 'tag'` the
moment a backend recorded a turn -- and a backend records on EVERY retained turn, so id
retention was unusable for such a subclass while `add()` on the same context worked fine.

Now built the same way as its neighbours: `type(self).__new__`, `Context.__init__`, then
the `_propagated_fields` copy. The return type narrows from `ChatContext` to `Self`,
which is what it was already returning at runtime.

Test watched failing first (with the `TypeError` above) in
test_recording_ids_keeps_a_subclass_with_required_ctor_args.

Assisted-by: Claude Code
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: noaa <noaa.kless@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: add context typing so that functions return the type of context passed into them

3 participants