Repository navigation
Python: fix(python): preserve Annotated metadata in kernel functions - #14496
Anvit Devadiga (AnvitDevadiga) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved issues were identified, and the regression coverage validates the fix.
Review effort: Lite
Findings: None
What changed in this PR
Preserves caller-owned Annotated metadata when parsing kernel function parameters.
Changes:
- Copies metadata dictionaries before removing
description. - Adds regression coverage for shared metadata.
| File | Description |
|---|---|
python/semantic_kernel/functions/kernel_function_decorator.py |
Prevents mutation of shared annotation metadata. |
python/tests/unit/functions/test_kernel_function_decorators.py |
Verifies metadata remains unchanged and reusable. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@microsoft-github-policy-service agree |
PRABHU KIRAN VANDRANKI (VANDRANKI)
left a comment
There was a problem hiding this comment.
Community review, so this does not clear the merge gate.
I read the full diff and _parse_parameter in python/semantic_kernel/functions/kernel_function_decorator.py on main. I also reproduced the bug in a local venv with semantic_kernel installed (a version without this fix). I did not run the PR's test.
Reproduction: one dict d = {"description": "the answer count"} used as Annotated[int, d] on two decorated functions. After decorating both, d was {}, the first function's parameter had the description, and the second function's parameter had description of None. The cause is meta.pop("description", None) mutating the caller's dict, which Annotated keeps by reference. So the bug is real and the symptom is order dependent, which makes it nasty to diagnose.
The fix, meta = meta.copy() before the pop, is correct and minimal. It is a shallow copy, which is enough because only a top-level key is popped. Values like nested dicts in ret.update(meta) are still shared with the caller's dict, but nothing mutates them here.
The regression test uses the same shared-dict pattern as my reproduction and checks both functions plus the original dict, so it covers the failure.
One thing I noticed while tracing, not caused by this PR: the line if "description" not in ret and (description := meta.pop("description", None)) short-circuits when ret already has a description (for example from an earlier string metadata item). In that case the dict's "description" is not popped, and the following ret.update(meta) overwrites the earlier description anyway. So the comment "only override from the metadata if it is not already set" is not what the code does in that ordering. That is a separate issue and I did not test it end to end.
Looks good to me for what it sets out to fix.
Fixes #14441\n\n## Summary\n- copy Annotated dictionary metadata before extracting the description\n- preserve caller-owned metadata when the same annotation is reused\n- add a regression test covering repeated decorated functions\n\n## Validation\n- focused kernel function decorator tests: 32 passed\n- Ruff checks passed\n