Repository navigation
Fix merge_keys case-sensitivity bug in build_context - #51
Merged
Merged
Conversation
…tion test
update_context lowercased the context key before looking it up in
composition.merge_keys, but merge_keys is a user-authored dict with no case
normalization, so a mixed-case key like myKey with merge_keys = {"myKey":
"update"} silently fell back to the comprehensive strategy. Drop the
.lower() call and document that merge key matching is case-sensitive.
Also adds a real (non-mocked) build_context test that interleaves two
overlays and a task with a conflicting mixed-case merge key, closing the
mock-only coverage gap called out in #44.
Fixes #44
Contributor
|
Contributor
|
Version hint: patch Comment ID: Display version hint-auto-generated |
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
1 participant
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.
Summary
update_contextlowercased the context key before looking it up incomposition.merge_keys, butmerge_keysis a user-authored dict with no case normalization — a mixed-case key likemyKeywithmerge_keys = {"myKey": "update"}silently fell back to thecomprehensivestrategy instead of the one configured. Dropped the.lower()call and documented that merge key matching is case-sensitive.build_contextintegration test that interleaves two overlays and a task with a conflicting mixed-case merge key, closing the mock-only coverage gap for this seam.Fixes #44
Test plan
uv run pytest --agent-digest=term -p no:sugar— 329 passedruff check/ruff format --checkclean/code-review medium— no findings