Summary
SamplingStrategy._merge_requirements resolves strategy-level requirements against per-call requirements by supersede (one list wins, the other is dropped), not by merge. This contradicts the method's name and the user-facing sample() docstring, both of which describe the two lists as merged. The mismatch has existed since sampling was first added and appears to be a long-standing implementation bug rather than a deliberate design.
Current behavior
mellea/core/sampling.py _merge_requirements: if self.requirements (strategy-level) is set, it is used and the per-call requirements are dropped entirely; only when the strategy has none are per-call requirements used. The two lists are never combined — dict.fromkeys only dedups whichever single list won.
Documented / intended behavior
The sample() requirements parameter has been documented as "merged with global requirements" since the first sampling commit, and the resolution method is named _merge_requirements. A union (both lists enforced, deduped) matches that documented intent and is the least-surprising behavior for a validation feature: no requirement the caller configured — on the call or on the strategy — is silently skipped.
History (blameless, by PR)
Impact
A caller who sets requirements on the strategy object and passes requirements at the call site silently loses one of the two lists. For a validation/requirements feature this is a quiet correctness footgun — a requirement the user asked for is not enforced, with no error or warning.
Proposed fix
Change _merge_requirements to union both lists (deduped, order-preserving), then align the class attribute docstring, the sample() param docstring, and the method behavior so all three agree on merge semantics.
Scope / risk
This changes released behavior for act()/aact()/instruct()/sample(). Supersede has shipped for ~13 months, so some usage may depend on it; the change may warrant a deprecation note or maintainer discussion before merging, rather than a silent flip.
Related
Streaming PR #1697 independently implements union for stream(strategy=...) at the requirement-clone site, so it does not depend on this fix. This issue reconciles the core sampling path to the same (intended) merge semantics.
Summary
SamplingStrategy._merge_requirementsresolves strategy-level requirements against per-call requirements by supersede (one list wins, the other is dropped), not by merge. This contradicts the method's name and the user-facingsample()docstring, both of which describe the two lists as merged. The mismatch has existed since sampling was first added and appears to be a long-standing implementation bug rather than a deliberate design.Current behavior
mellea/core/sampling.py_merge_requirements: ifself.requirements(strategy-level) is set, it is used and the per-callrequirementsare dropped entirely; only when the strategy has none are per-call requirements used. The two lists are never combined —dict.fromkeysonly dedups whichever single list won.Documented / intended behavior
The
sample()requirementsparameter has been documented as "merged with global requirements" since the first sampling commit, and the resolution method is named_merge_requirements. A union (both lists enforced, deduped) matches that documented intent and is the least-surprising behavior for a validation feature: no requirement the caller configured — on the call or on the strategy — is silently skipped.History (blameless, by PR)
_merge_requirements, faithfully preserving supersede and carrying both contradictory docstrings forward.Impact
A caller who sets requirements on the strategy object and passes requirements at the call site silently loses one of the two lists. For a validation/requirements feature this is a quiet correctness footgun — a requirement the user asked for is not enforced, with no error or warning.
Proposed fix
Change
_merge_requirementsto union both lists (deduped, order-preserving), then align the class attribute docstring, thesample()param docstring, and the method behavior so all three agree on merge semantics.Scope / risk
This changes released behavior for
act()/aact()/instruct()/sample(). Supersede has shipped for ~13 months, so some usage may depend on it; the change may warrant a deprecation note or maintainer discussion before merging, rather than a silent flip.Related
Streaming PR #1697 independently implements union for
stream(strategy=...)at the requirement-clone site, so it does not depend on this fix. This issue reconciles the core sampling path to the same (intended) merge semantics.