fix(ers): continue failure_strategy bug - first-match-wins per ADR - #3964
Open
elizabethhealy wants to merge 2 commits into
Open
fix(ers): continue failure_strategy bug - first-match-wins per ADR#3964elizabethhealy wants to merge 2 commits into
elizabethhealy wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (11)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughMulti-Strategy entity resolution now stops after the first successful strategy. Contract tests support implementation-specific chain shapes. Unit, integration, and BDD tests validate single-entity chains, strategy ordering, and failure fall-through. ChangesMulti-Strategy first-match resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to Multi-strategy entity resolution now returns only the first successful strategy result, with later strategies used only after a failure under continue mode. The behavior and associated chain-shape expectations are covered across runtime and regression tests, with no remaining merge-blocking risk identified. Sequence Diagram(s)sequenceDiagram
participant Token
participant MultiStrategy
participant EntityChain
Token->>MultiStrategy: Submit token
MultiStrategy->>MultiStrategy: Try strategies in configured order
MultiStrategy->>EntityChain: Store first successful entity
MultiStrategy->>MultiStrategy: Continue only after failure
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 68.42% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 8 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
elizabethhealy
changed the title
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
elizabethhealy
changed the title
elizabethhealy
changed the title
elizabethhealy
changed the base branch from
main
to
feat/dspx-4581-ers-bdd-gaps
September 4, 2026 14:16
elizabethhealy
marked this pull request as ready for review
September 4, 2026 14:16
elizabethhealy
changed the base branch from
feat/dspx-4581-ers-bdd-gaps
to
main
September 4, 2026 14:29
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
|
elizabethhealy
changed the base branch from
main
to
feat/dspx-4581-ers-bdd-gaps
September 4, 2026 14:46
Contributor
| // First match wins, per the ADR: failure_strategy only governs error handling | ||
| // ("continue" tries the next strategy on failure, "fail-fast" stops immediately). | ||
| // Neither mode keeps resolving after a success, so a chain holds exactly one entity. | ||
| break |
Member
There was a problem hiding this comment.
can make every decision fail for environment-first configs.
I am verifying now...
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
2 participants
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.
Proposed Changes
continuekept running all matching strategies after a success and accumulated one entity per
strategy into the chain. The ADR is explicit
that on success, execution stops regardless of failure strategy; a token's entity chain
always holds exactly one entity
continuestill does what it's supposed to — the ADR definescontinueas error handling only: if a strategy fails, fall through to the next matching one.
The fix only changes what happens on success
listed first in config and matches the token is the one that resolves it
behavior; it now correctly asserts DENY and acts as a regression guard
Note for reviewers
The customer pattern of "Claims for routing, LDAP for department" across two separate
strategies is not supported — multi-entity chain composition is explicitly deferred in
the ADR as future work.
The workaround is to put the richer strategy first, or merge both sources in a single
strategy's
output_mapping.Checklist
Testing Instructions
Summary by CodeRabbit
Behavior Changes
Bug Fixes
Documentation