[grid] Fix DefaultSlotMatcher matching requests differentiated only by undeclared automationName - #17898
[grid] Fix DefaultSlotMatcher matching requests differentiated only by undeclared automationName#17898diemol wants to merge 5 commits into
Conversation
…ionName extensionCapabilitiesMatch() only inspected extension capability names the stereotype declared, so a stereotype declaring none at all (e.g. a plain browser node) matched any requested automationName by default. This let a native-automation request differentiated solely by automationName match an unrelated browser-only node. Add automationNameMatch(), gated on the stereotype showing some existing Appium-awareness (a relevant extension capability, or a non-W3C-compliant platformVersion), so relay-node matching keeps working while a stereotype with no such awareness no longer matches on an undeclared automationName. Fixes #17845 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PR Summary by QodoGrid: Prevent slot matching on undeclared appium:automationName
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
Fixes two issues found in CI/review of #17898: - Format / Check Format was failing on ImmutableCapabilities argument wrapping in the new tests; re-ran ./go format to match. - Qodo correctly flagged that treating any non-vendor extension capability as "Appium-aware" was too broad -- a node advertising an unrelated custom extension capability (e.g. prefixed:cheese) would incorrectly bypass the automationName gate. Narrow the signal to appium:-prefixed capabilities and platformVersion specifically, and tighten the automationName check to an exact/suffix match instead of a bare substring. Added a regression test for the narrowed case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit e62d621 |
google-java-format wants the noneMatch lambda collapsed onto one line; the previous manual edit split it across two. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 97ccbef |
Per Qodo review feedback on #17898: document the public matches() method's parameters and return value, since its behavior changed with the new automationName gating. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Code review by qodo was updated up to the latest commit 0d2c0c8 |
🔗 Related Issues
Fixes #17845
💥 What does this PR do?
DefaultSlotMatcher.extensionCapabilitiesMatch()only inspects extension capability names the stereotype declares. If a stereotype declares no extension (:-namespaced) capabilities at all — e.g. a plain browser node — the check short-circuits totrueregardless of what the request asks for. This lets a request differentiated solely by an identity extension capability (appium:automationName) match a completely unrelated, non-Appium node, as long asbrowserNameis absent from the request andplatformNameresolves to the samePlatformfamily.This adds a new
automationNameMatch()check, called alongside the existing extension-capability checks inmatches(). It only rejects a match when the stereotype shows no existing Appium-awareness at all (no relevant extension capability, and no non-W3C-compliantplatformVersion, which the code already treats as an Appium signal) while the request specifies anautomationName. Stereotypes that already show some Appium-awareness (e.g. relay nodes advertisingappium:platformVersionbut omittingautomationName) are left untouched, preserving existing relay-node matching behavior.🔧 Implementation Notes
The issue proposed an unconditional bidirectional
automationNamecheck. That approach was tested against the existing test suite and found to break two passing relay-node tests, where Appium relay nodes intentionally omitautomationNamewhile still being valid targets for varied automation sessions. The fix here gates the check on stereotype Appium-awareness instead, using the sameplatformVersionsignal the codebase already relies on elsewhere in this file, so relay-node flexibility is preserved while the reported misroute is closed.🤖 AI assistance
automationNameMatch()and the two regression tests, drafted from the issue's analysis and root-caused/iterated against the existing test suite to avoid regressing relay-node matching.💡 Additional Considerations
None.
🔄 Types of changes