feat: treat a missing parameter as unsatisfied instead of raising - #46
feat: treat a missing parameter as unsatisfied instead of raising#46smandegar wants to merge 1 commit into
Conversation
A feature that references several parameters can now be evaluated with only a subset supplied: an absent parameter leaves its condition unsatisfied rather than raising. Supplying no parameters at all still raises.
diff-vader-bot
left a comment
diff-vader-bot
left a comment
There was a problem hiding this comment.
🟢 Risk: LOW — no substantive findings
✅ Diff Vader · Approved
Council reviewed 5 files · 12 reviewers · 0 findings · $0.36
- ✅
blast‑radius - ✅
correctness - ✅
cost - ❓
github‑actions— skipped: filter: not applicable to changed files - ❓
iac— skipped: filter: not applicable to changed files - ❓
migration‑safety— skipped: filter: not applicable to changed files - ✅
performance - ✅
revertibility - ✅
security‑tenant‑isolation - ❓
supply‑chain— skipped: filter: not applicable to changed files - ✅
supportability - ✅
test‑adequacy
Why: All approval gates passed.
💡 /diff-vader-review <name> adds a one-shot reviewer. Beyond this PR's council, you can request: backup-correctness, loop-design-system.
|
|
||
| value = parameters[condition.parameter.to_sym] | ||
| raise ArgumentError, "Missing parameter #{condition.parameter}" if value.nil? | ||
| if value.nil? |
There was a problem hiding this comment.
pr-reviewer-blast-radius — confidence 85% (advisory)
This changes the evaluation contract: a condition whose parameter is absent from a non-empty bag is now silently treated as unsatisfied instead of raising. Callers that previously relied on the Missing parameter <name> ArgumentError to detect a specific absent parameter will now get a silent false (feature-off) instead. The PR body explicitly acknowledges this behavioral change and its impact on such callers, so this is a triaged, deliberate change rather than a novel defect — flagging only so downstream consumers are aware the feature may now evaluate to disabled where it previously errored.
React with 👍 / 👎 / 😕 to help us calibrate — why.
| later = EightBall::Conditions::List.new parameter: 'account_id', values: [1, 2] | ||
|
|
||
| feature = EightBall::Feature.new 'Feature', [satisfied, later] | ||
|
|
There was a problem hiding this comment.
pr-reviewer-test-adequacy — confidence 85% (advisory)
The should short-circuit before reaching a later condition with an unsupplied parameter test calls feature.enabled? with no parameters, which means the early short-circuit is handled by the EightBall::Conditions::Always path (parameter is nil, so it returns immediately). This doesn't cover the case where a non-parameterized condition short-circuits before a parameterized one when an empty bag is supplied — but that's arguably out of scope. More importantly, the test doesn't verify that omitting parameters when the first condition also requires a parameter (non-short-circuit path) still returns false. This is already covered by other tests, so no gap. Advisory: does this test actually exercise the next false branch introduced in any_satisfied?, or only the return condition.satisfied? branch for parameter-nil conditions?
React with 👍 / 👎 / 😕 to help us calibrate — why.
There was a problem hiding this comment.
Pull request overview
This PR changes EightBall::Feature#enabled? evaluation so that when a condition references a parameter that is not present in a non-empty parameters hash, that condition is treated as unsatisfied (instead of raising). An empty parameters hash (or calling enabled? with no args, which defaults to {}) still raises when evaluation reaches a parameterized condition.
Changes:
- Update feature evaluation to treat missing parameters in a non-empty bag as “unsatisfied”, while preserving an
ArgumentErroron empty-parameter evaluation. - Refresh specs to assert the new behavior, including the empty-parameters error contract.
- Update README and CHANGELOG to reflect the behavioral change.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/eight_ball/feature.rb | Adjusts condition evaluation behavior for missing vs empty parameters and updates YARD docs. |
| spec/feature_spec.rb | Updates and expands unit coverage for the new missing-parameter semantics. |
| spec/stress_spec.rb | Updates contract-oriented stress/spec assertion to match the new behavior. |
| README.md | Updates documentation for parameterized condition behavior (percentage). |
| CHANGELOG.md | Records the behavior change under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| conditions.any? do |condition| | ||
| return condition.satisfied? if condition.parameter.nil? | ||
|
|
| # @raise [ArgumentError] if +parameters+ is empty and one of the Feature's | ||
| # {EightBall::Conditions} requires a parameter. A condition whose parameter | ||
| # is simply absent from a non-empty +parameters+ is treated as unsatisfied. |
| it 'should short-circuit before reaching a later condition with an unsupplied parameter' do | ||
| satisfied = EightBall::Conditions::Always.new | ||
| later = EightBall::Conditions::List.new parameter: 'account_id', values: [1, 2] | ||
|
|
||
| feature = EightBall::Feature.new 'Feature', [satisfied, later] | ||
|
|
||
| expect(feature.enabled?).to be true | ||
| end | ||
|
|
||
| it 'should not raise for a bare feature evaluated with an empty parameters hash' do | ||
| feature = EightBall::Feature.new 'NoConditions' | ||
|
|
||
| expect(feature.enabled?({})).to be true | ||
| end |
What
Feature#enabled?no longer raises when a condition references a parameter that isn't present in the supplied parameters. That condition is treated as unsatisfied instead, so evaluation continues normally (OR acrossenabledFor, anddisabledForstill wins).Supplying no parameters at all still raises
ArgumentError("At least one parameter is required"), so a call that forgets to pass parameters is still caught.Why
A feature can reference several parameters (for example one condition on
planand another onaccount_id), and different call sites may only know a subset of them. Previously, evaluating such a feature from a call site that supplied only some of its parameters raised, even when the supplied parameters were already enough to decide the result. Now a caller can pass just the parameters it has; conditions it cannot evaluate simply do not match.Behavior
always/never) and a short-circuiting earlier match are unaffected.Notes
This is a behavioral change: callers that relied on the previous per-parameter "Missing parameter" raise to detect a specific absent parameter will no longer get it (except for the empty-parameters case). Tests and CHANGELOG are updated.