fix: assign the result of Context.add in transform/atransform - #1477
fix: assign the result of Context.add in transform/atransform#1477planetf1 wants to merge 1 commit into
Conversation
Context.add is non-mutating, but new_ctx.add(chosen_tool) in transform() and atransform() was called as a bare statement, discarding the returned context. The chosen tool message never reached the context handed back to callers, despite an info log claiming it had been added. Assisted-by: Claude Code Signed-off-by: Nigel Jones <jonesn@uk.ibm.com>
AngeloDanducci
left a comment
AngeloDanducci
left a comment
There was a problem hiding this comment.
Overall LGTM, one non blocking enhancement.
| _, new_ctx = transform(MObject(), "transform it", ctx, MagicMock()) | ||
|
|
||
| assert tool_message in new_ctx.as_list() |
There was a problem hiding this comment.
May be nice to verify ordering? Not a blocker though.
| _, new_ctx = transform(MObject(), "transform it", ctx, MagicMock()) | |
| assert tool_message in new_ctx.as_list() | |
| _, new_ctx = transform(MObject(), "transform it", ctx, MagicMock()) | |
| # The tool message must be present and land as the most-recent turn. | |
| assert tool_message in new_ctx.as_list() | |
| assert new_ctx.as_list()[-1] is tool_message |
There was a problem hiding this comment.
Good suggestion to verify order and I would add also that it should verify that add was additive and did not clobber the initial contents.
Unfortunately I think the current mock setup makes the result a list of one (it is added to empty) so it does not really verify order or additive.
This concern also makes me think this might be more than an nit/good-idea and actually quite valuable.
assert len(new_ctx.as_list()) > 1
assert tool_message == new_ctx.as_list()[-1]
markstur
left a comment
markstur
left a comment
There was a problem hiding this comment.
good 2-line fix
Test fails before fix and passes after 🆒
Testing order is a good idea. I'd add that it needs to also test that we are adding to a non-empty list.
| _, new_ctx = transform(MObject(), "transform it", ctx, MagicMock()) | ||
|
|
||
| assert tool_message in new_ctx.as_list() |
There was a problem hiding this comment.
Good suggestion to verify order and I would add also that it should verify that add was additive and did not clobber the initial contents.
Unfortunately I think the current mock setup makes the result a list of one (it is added to empty) so it does not really verify order or additive.
This concern also makes me think this might be more than an nit/good-idea and actually quite valuable.
assert len(new_ctx.as_list()) > 1
assert tool_message == new_ctx.as_list()[-1]
| """ | ||
| from mellea.stdlib.functional import transform | ||
|
|
||
| ctx = ChatContext() |
There was a problem hiding this comment.
If testing order and non-clobbering of existing entries. Maybe this?
ctx = ChatContext().add(Message("user", "test"))
3 participants
Pull Request
Issue
Fixes #1475
Description
transform()andatransform()both callnew_ctx.add(chosen_tool)as a bare statement.Context.addis non-mutating, so the returned context is discarded and the chosen tool message never reaches the context handed back to the caller — despite an info log claiming it was added.Fix: assign the result at both call sites (
mellea/stdlib/functional.py:502intransform(),:1271inatransform()).Testing
Attribution
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.
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.