Short-circuit if we have a v2 capability#22356
Conversation
product-security-plaid-production
Bot
requested review from
justinkaseman and
nolag
|
👋 cedric-cordenier, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
cedric-cordenier
force-pushed
the
CRE-4087-short-circuit-v2-caps
branch
from
2ca24cb to
a5393d8
Compare
|
I see you updated files related to
|
| if errAdd != nil { | ||
| return fmt.Errorf("failed to add remote v2 capability %s: %w", capability.ID, errAdd) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Won't this break cron? It's used by both, but was upgraded to v2.
There was a problem hiding this comment.
CRON is not a remote capability.
We only care about chain caps and vault here and they all have methodConfig set.
There was a problem hiding this comment.
Pull request overview
Risk Rating: MEDIUM
This PR fixes the remote capability registration flow by ensuring that once a v2 capability is successfully added via the CombinedClient path, the launcher does not continue into the v1 shim-registration logic, preventing duplicate/incorrect registration attempts.
Changes:
- Add an early
return nilafter successfully adding a remote v2 capability to short-circuit further processing.
Areas requiring scrupulous human review:
addRemoteCapabilityv2 detection/branching: confirm thatcapabilityConfig.CapabilityMethodConfig != nilis the definitive signal for “v2 capability” across all supported configurations, and that no v2 capability should ever also go through the v1switch capability.CapabilityTypeblock.
|
✅ No conflicts with other open PRs targeting |
| if errAdd != nil { | ||
| return fmt.Errorf("failed to add remote v2 capability %s: %w", capability.ID, errAdd) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
CRON is not a remote capability.
We only care about chain caps and vault here and they all have methodConfig set.
|
Requires
Supports