[adr] network handler behavior proposal

[titusfortner]

Last Fall we were discussing @p0deje's BiDi Protocol Design Document. If you read the comments on that doc, you'll see that @nvborisenko and I raised a bunch of concerns and suggested improvements. I did not submit a full counter proposal like I planned to, and we stopped having meetings to discuss it so nothing was resolved.

Binding Status

Binding	Status	Notes
Python	Incomplete	Reconcile by fixed priority (fail > provide_response > continue_response) with a continue_response override, rather than first-disposition (1,2) response has no fail (1); dispatch is FIFO (3); Sends mutated event when errors instead of discarding it (4); original event is not accessible, only mutated event (6); no complete implementation (7); body not collected behind the handler (8).
Ruby	Incomplete	Handlers run in parallel threads, so multi-handler disposition is currently a race (1,2,3) and exception logging (4, 5). Missing: no default-continue (2), FIFO not LIFO (3), no preserved original (6), no complete (7), user-managed body (8)
Java	Incomplete	Only first matching handler runs, so no reconciliation, and disposition is always continue (1,2,3); A throwing handler propagates and leaves event blocked (4); currently return driven (5); no complete implementation (7), user-managed body (8); also no AddResponseHandler
.NET	Incomplete	no AddRequestHandler/AddResponseHandler
JavaScript	Incomplete	no AddRequestHandler/AddResponseHandler

💥 What does this PR do?

This ADR represents my counter proposal to the document above, focusing on behaviors instead of implementation details (though an implementation I sketched out in Ruby does all these things).

I've spent a lot of time looking at edge cases and comparing things to Playwright to provide what I think is the best approach.

🔧 Implementation Notes

• These are all behaviors that we should nail down early because they are not backwards compatible
• These behaviors can be implemented in multiple ways and the implementation doesn't need to be prescribed so long as these behaviors are supported
• This defers everything that is an "implementation detail" and not focused on user facing behavior/API
• There are a few things I've specified as out of scope for this ADR
• I have several follow on proposals to further these items if we agree to them.

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s): Claude
  • What was generated:
  • I reviewed all AI output and can explain the change

💡 Additional Considerations

• I'm doing this as one ADR because many of them are tied together, but that means that there are a lot of things that can be partially agreed to here and lots of potential conversations and disagreements.

🔄 Types of changes

• Cleanup (formatting, renaming)
• Bug fix (backwards compatible)
• New feature (non-breaking change which adds functionality and tests!)
• Breaking change (fix or feature that would cause existing functionality to change)

[qodo-code-review]

PR Summary by Qodo

ADR: Define network handler disposition, ordering, and failure semantics
📝 Documentation 🕐 20-40 Minutes

Description

• Proposes user-facing behaviors for resolving conflicting network handlers.
• Defines handler disposition, default chaining, LIFO ordering, and exception handling.
• Specifies completion capture and body collection responsibilities for handlers.

Diagram

graph TD
U(["User/Test"]) --> API["Network API"] --> HS["Handler stack"] --> HC["Handler callable"] --> D{"Disposition?"}
D --> F["Fail request"] --> B["BiDi command"]
D --> R["Provide response"] --> B
D --> S["Continue/Submit"] --> B
HC --> E["Log & skip"]
HC --> C[("Completed event")]

subgraph Legend
  direction LR
  _actor(["Actor"]) ~~~ _proc["Process"] ~~~ _dec{"Decision"} ~~~ _store[("Stored state")]
end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Require explicit disposition (Playwright-style fallback)

• ➕ Avoids ambiguity: every handler must declare whether it stops or continues processing
• ➕ Reduces surprises when handlers accidentally “chain” due to omitted disposition
• ➖ More verbose for common “mutate and continue” cases (headers, URL rewrites, etc.)
• ➖ Makes composition of multiple small handlers more cumbersome

2. Run all handlers then reconcile by fixed priority

• ➕ Deterministic resolution even if multiple handlers set conflicting dispositions
• ➕ Potentially simpler mental model for some users (priority table)
• ➖ Prevents a specific handler from intentionally short-circuiting others
• ➖ Can unintentionally apply mutations from handlers that “should not have run” under a user-chosen stop condition

3. FIFO ordering (first registered wins)

• ➕ Matches many event-listener models where registration order implies precedence
• ➕ Potentially easier to reason about for simple setups
• ➖ Makes local overrides difficult (tests can’t reliably override shared/global handlers)
• ➖ Encourages brittle global configuration and test coupling

Recommendation: The ADR’s proposed model (explicit disposition when needed, default chaining, LIFO precedence, and exception isolation) is the most composable for real test suites where local overrides must trump shared defaults. The main point to validate with stakeholders is whether the default “continue to next handler” is acceptable ergonomically and safety-wise; if not, consider the explicit-fallback alternative, but expect materially more boilerplate across bindings.

Files changed (1) +230 / -0

Documentation (1) +230 / -0

network-handler-behavior.mdAdd ADR defining network handler behavior and precedence rules +230/-0

Add ADR defining network handler behavior and precedence rules

• Introduces a proposed ADR that specifies how request/response handlers compose and resolve conflicts. Defines disposition verbs, default chaining behavior, LIFO ordering, exception handling, ignoring return values, access to original vs mutated event state, completion capture, and body-collection responsibilities.

docs/decisions/network-handler-behavior.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (5) 📘 Rule violations (0) 📜 Skill insights (0)

1. Wrong Python behavior claim 🐞 Bug ≡ Correctness ⭐ New

Description

The ADR claims current Python behavior lets continueRequest override failures and stubs, but
Python’s BiDi network handler registry resolves outcomes with fixed precedence where any fail()
wins and provide_response() wins over mutations. This misstates the existing semantics and can
mislead readers about what is actually changing.

Code

docs/decisions/17685-network-handler-behavior.md[R158-159]

+  - We could run every handler but have `continueRequest` override failures and stubs (current
+    Python behavior), but it is not obvious why that command should have precedence.

Evidence

Python’s handler registry documents and implements reconciliation where any handler calling fail()
causes the request to be failed during _resolve(), and the integration tests assert that fail
wins when handlers disagree—so continueRequest does not override failures/stubs in current Python
behavior.

py/private/_network_handlers.py[28-37]
py/private/_network_handlers.py[428-437]
py/test/selenium/webdriver/common/bidi_network_tests.py[251-266]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The ADR’s “Considered options → Reconciliation” section states that in current Python behavior `continueRequest` overrides failures and stubs. The Python implementation instead reconciles handler actions with a strict priority (fail > provide_response > continue-with-mutations/default continue), so the ADR’s comparison baseline is inaccurate.

## Issue Context
This ADR is intended to guide cross-binding behavior decisions; incorrect statements about existing behavior can skew review/consensus and lead to incorrect implementations.

## Fix Focus Areas
- docs/decisions/17685-network-handler-behavior.md[154-160]

## Suggested change
Reword the parenthetical to reflect the actual Python semantics (e.g., “current Python behavior reconciles outcomes with fixed priority (fail > provide_response > continue) after running all matching handlers”).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Auth callable contradiction 🐞 Bug ≡ Correctness ⭐ New

Description

The ADR excludes authentication handlers because “authentication should not use a callable,” but
Selenium Ruby’s add_authentication_handler explicitly accepts a block and existing tests use
block-based auth handlers (&:skip, &:cancel). This makes the ADR’s rationale inaccurate for at
least one binding and risks confusing implementers about the scope/baseline.

Code

docs/decisions/17685-network-handler-behavior.md[R20-21]

+This applies to request and response handlers, but not authentication handlers, since
+authentication should not use a callable.

Evidence

Ruby’s Network API accepts an optional block for add_authentication_handler and uses it when
credentials aren’t provided, and Ruby integration tests exercise callable auth handlers via &:skip
and &:cancel, contradicting the ADR’s claim that authentication should not use a callable.

rb/lib/selenium/webdriver/common/network.rb[47-63]
rb/spec/integration/selenium/webdriver/network_spec.rb[86-101]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The ADR states authentication handlers are out of scope because authentication should not use a callable. In current Selenium Ruby, authentication handlers can be provided as a callable/block, so the statement is not universally true across bindings.

## Issue Context
The ADR is meant to be cross-binding guidance; blanket statements that contradict an existing binding’s public API are likely to cause confusion.

## Fix Focus Areas
- docs/decisions/17685-network-handler-behavior.md[20-21]

## Suggested change
Reword to scope the statement as a proposal or explicitly note that auth handler behavior is out of scope and that existing bindings differ (e.g., “This ADR does not cover authentication handlers; bindings currently vary, and we may revisit whether auth should be callable-based separately.”).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Overgeneralized exception behavior 🐞 Bug ⚙ Maintainability

Description

The ADR states (in a comparative bullet) that Selenium logs uncaught handler exceptions instead of
ending the session, but current behavior is binding-specific (e.g., Ruby handler blocks are executed
without rescue/logging, while Python does catch/log). This should be reworded as a proposed
cross-binding behavior or explicitly scoped to the bindings that implement it today.

Code

docs/decisions/network-handler-behavior.md[R77-80]

+   * In Playwright, uncaught exceptions propagate to end the session, which causes problems when
+     something unrelated to the test's intent goes wrong.
+   * Selenium is more lenient and only logs the error to the console.
+

Evidence

Ruby currently invokes handler blocks without rescue/logging, while Python explicitly catches and
logs exceptions, so the ADR’s statement is not uniformly true across Selenium bindings today.

docs/decisions/network-handler-behavior.md[77-80]
rb/lib/selenium/webdriver/common/network.rb[89-96]
py/private/_network_handlers.py[746-756]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The ADR text implies a universal Selenium behavior (“only logs the error to the console”) for handler exceptions, but current bindings differ.

### Issue Context
Ruby’s handler dispatch yields directly to user code without rescue; Python catches/logs and continues.

### Fix Focus Areas
- docs/decisions/network-handler-behavior.md[72-80]

### What to change
- Rephrase the bullet to one of:
 - “In some Selenium bindings (e.g., Python), uncaught handler exceptions are logged and processing continues…”
 - or “Proposed: Selenium should log uncaught handler exceptions and continue processing…”
- If desired, add a short note indicating current binding differences to avoid readers assuming uniform behavior today.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (2)

4. Wrong handler verb names 🐞 Bug ≡ Correctness

Description

The ADR states Selenium request/response handlers use respond/submit (and response fail), but
existing bindings expose different verbs (e.g., Python provide_response/continue_request, Ruby
provide_response/continue) and do not offer a response-level fail. This mismatch will mislead
implementers/readers about current APIs and what exactly is being proposed to change.

Code

docs/decisions/network-handler-behavior.md[R31-43]

+   * Selenium supports:
+     * Request: `fail` (Playwright's `abort`, BiDi's `FailRequest`), `respond` (Playwright's `fulfill`, BiDi's `ProvideResponse`), and `submit` (Playwright's `continue`, BiDi's `ContinueRequest`).
+     * Response: `fail` (BiDi's `FailRequest`), and `submit`: note that since we don't need to prevent a round trip from a request, whether this is a BiDi `ContinueResponse` or `ProvideResponse` can be an implementation detail based on whether a replacement body value is provided.
+
+```ruby
+# Specifics of parameters and names can match spec details
+network.add_request_handler { |r| r.fail if something }
+network.add_request_handler { |r| r.respond(content: mocked_response) if something }
+network.add_request_handler { |r| r.add_header("X-Test", true) && r.submit if something }
+
+network.add_response_handler { |r| r.fail if something }
+network.add_response_handler { |r| r.submit(content: mocked_response) if something }
+network.add_response_handler { |r| r.add_header("X-Test", true) && r.submit if something }

Evidence

The ADR’s listed verbs don’t exist in current binding implementations, which use different method
names and capabilities for request/response interception.

docs/decisions/network-handler-behavior.md[31-43]
py/private/_network_handlers.py[325-438]
py/private/_network_handlers.py[440-581]
rb/lib/selenium/webdriver/bidi/network/intercepted_request.rb[30-58]
rb/lib/selenium/webdriver/bidi/network/intercepted_response.rb[31-68]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The ADR uses handler disposition verbs (`respond`, `submit`, and response `fail`) that don’t match the verbs exposed by current Selenium bindings, which makes the document confusing as a proposal baseline.

### Issue Context
Current Python and Ruby bindings use `provide_response`/`continue_request` (Python) and `provide_response`/`continue` (Ruby), and do not expose a response-level `fail`.

### Fix Focus Areas
- docs/decisions/network-handler-behavior.md[26-44]

### What to change
- Replace “Selenium supports:” wording with either:
 - a binding-neutral set of verbs (and explicitly state they are *proposed* names), or
 - the currently used names (and optionally add a mapping table: proposed ↔ existing per binding).
- Update the Ruby/Python code examples accordingly (e.g., `provide_response`/`continue_request`/`continue`) and remove or clearly label response `fail` as a proposed addition if that’s intended.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Example violates stated behavior 🐞 Bug ≡ Correctness

Description

The Appendix claims the sample process_request implementation satisfies the ADR behaviors, but it
does not handle handler exceptions (behavior 4) and would propagate exceptions from
h.call(request) instead of logging and continuing. As written, it also cannot discard per-handler
staged mutations on exception because it mutates a shared request object.

Code

docs/decisions/network-handler-behavior.md[R208-226]

+The behaviors in this ADR explicitly do not specify an implementation. For illustrative
+purposes, this code — with state stored in the request wrapper object and evaluated after
+execution inside the loop — will satisfy the above behaviors:
+
+```ruby
+def process_request(request)
+  @handlers.reverse_each do |h|
+    h.call(request)
+    if request.complete?
+      h.request = request
+      remove_handler(h)
+    end
+    if request.failed?
+      return fail_request(request)
+    elsif request.response?
+      return provide_response(request)
+    elsif request.submit? || request.complete?
+      return continue_request(request)
+    end

Evidence

The ADR requires log-and-continue on handler exceptions, but the appendix code calls handlers
without any rescue/try-catch while still claiming it satisfies the behaviors.

docs/decisions/network-handler-behavior.md[72-79]
docs/decisions/network-handler-behavior.md[208-226]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The appendix implementation is presented as satisfying the ADR behaviors, but it lacks exception handling around handler invocation and does not show how staged mutations are discarded when a handler errors.

### Issue Context
Behavior 4 requires uncaught exceptions to be logged, staged changes discarded, and processing to continue as if the handler was not registered.

### Fix Focus Areas
- docs/decisions/network-handler-behavior.md[72-85]
- docs/decisions/network-handler-behavior.md[208-229]

### What to change
- Wrap `h.call(request)` in a begin/rescue (or language-appropriate equivalent) and explicitly note logging + continuing.
- Either:
 - update the pseudocode to show per-handler isolation/rollback of staged mutations (e.g., snapshot/clone before calling handler and restore on exception), or
 - amend the text to state the pseudocode is simplified and *does not* model rollback semantics (so it’s not claimed to fully satisfy behavior 4).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 18895d1

Results up to commit b09a2bc

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Wrong handler verb names 🐞 Bug ≡ Correctness

Description

The ADR states Selenium request/response handlers use respond/submit (and response fail), but
existing bindings expose different verbs (e.g., Python provide_response/continue_request, Ruby
provide_response/continue) and do not offer a response-level fail. This mismatch will mislead
implementers/readers about current APIs and what exactly is being proposed to change.

Code

docs/decisions/network-handler-behavior.md[R31-43]

+   * Selenium supports:
+     * Request: `fail` (Playwright's `abort`, BiDi's `FailRequest`), `respond` (Playwright's `fulfill`, BiDi's `ProvideResponse`), and `submit` (Playwright's `continue`, BiDi's `ContinueRequest`).
+     * Response: `fail` (BiDi's `FailRequest`), and `submit`: note that since we don't need to prevent a round trip from a request, whether this is a BiDi `ContinueResponse` or `ProvideResponse` can be an implementation detail based on whether a replacement body value is provided.
+
+```ruby
+# Specifics of parameters and names can match spec details
+network.add_request_handler { |r| r.fail if something }
+network.add_request_handler { |r| r.respond(content: mocked_response) if something }
+network.add_request_handler { |r| r.add_header("X-Test", true) && r.submit if something }
+
+network.add_response_handler { |r| r.fail if something }
+network.add_response_handler { |r| r.submit(content: mocked_response) if something }
+network.add_response_handler { |r| r.add_header("X-Test", true) && r.submit if something }

Evidence

The ADR’s listed verbs don’t exist in current binding implementations, which use different method
names and capabilities for request/response interception.

docs/decisions/network-handler-behavior.md[31-43]
py/private/_network_handlers.py[325-438]
py/private/_network_handlers.py[440-581]
rb/lib/selenium/webdriver/bidi/network/intercepted_request.rb[30-58]
rb/lib/selenium/webdriver/bidi/network/intercepted_response.rb[31-68]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The ADR uses handler disposition verbs (`respond`, `submit`, and response `fail`) that don’t match the verbs exposed by current Selenium bindings, which makes the document confusing as a proposal baseline.

### Issue Context
Current Python and Ruby bindings use `provide_response`/`continue_request` (Python) and `provide_response`/`continue` (Ruby), and do not expose a response-level `fail`.

### Fix Focus Areas
- docs/decisions/network-handler-behavior.md[26-44]

### What to change
- Replace “Selenium supports:” wording with either:
 - a binding-neutral set of verbs (and explicitly state they are *proposed* names), or
 - the currently used names (and optionally add a mapping table: proposed ↔ existing per binding).
- Update the Ruby/Python code examples accordingly (e.g., `provide_response`/`continue_request`/`continue`) and remove or clearly label response `fail` as a proposed addition if that’s intended.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Example violates stated behavior 🐞 Bug ≡ Correctness

Description

The Appendix claims the sample process_request implementation satisfies the ADR behaviors, but it
does not handle handler exceptions (behavior 4) and would propagate exceptions from
h.call(request) instead of logging and continuing. As written, it also cannot discard per-handler
staged mutations on exception because it mutates a shared request object.

Code

docs/decisions/network-handler-behavior.md[R208-226]

+The behaviors in this ADR explicitly do not specify an implementation. For illustrative
+purposes, this code — with state stored in the request wrapper object and evaluated after
+execution inside the loop — will satisfy the above behaviors:
+
+```ruby
+def process_request(request)
+  @handlers.reverse_each do |h|
+    h.call(request)
+    if request.complete?
+      h.request = request
+      remove_handler(h)
+    end
+    if request.failed?
+      return fail_request(request)
+    elsif request.response?
+      return provide_response(request)
+    elsif request.submit? || request.complete?
+      return continue_request(request)
+    end

Evidence

The ADR requires log-and-continue on handler exceptions, but the appendix code calls handlers
without any rescue/try-catch while still claiming it satisfies the behaviors.

docs/decisions/network-handler-behavior.md[72-79]
docs/decisions/network-handler-behavior.md[208-226]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The appendix implementation is presented as satisfying the ADR behaviors, but it lacks exception handling around handler invocation and does not show how staged mutations are discarded when a handler errors.

### Issue Context
Behavior 4 requires uncaught exceptions to be logged, staged changes discarded, and processing to continue as if the handler was not registered.

### Fix Focus Areas
- docs/decisions/network-handler-behavior.md[72-85]
- docs/decisions/network-handler-behavior.md[208-229]

### What to change
- Wrap `h.call(request)` in a begin/rescue (or language-appropriate equivalent) and explicitly note logging + continuing.
- Either:
 - update the pseudocode to show per-handler isolation/rollback of staged mutations (e.g., snapshot/clone before calling handler and restore on exception), or
 - amend the text to state the pseudocode is simplified and *does not* model rollback semantics (so it’s not claimed to fully satisfy behavior 4).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Overgeneralized exception behavior 🐞 Bug ⚙ Maintainability

Description

The ADR states (in a comparative bullet) that Selenium logs uncaught handler exceptions instead of
ending the session, but current behavior is binding-specific (e.g., Ruby handler blocks are executed
without rescue/logging, while Python does catch/log). This should be reworded as a proposed
cross-binding behavior or explicitly scoped to the bindings that implement it today.

Code

docs/decisions/network-handler-behavior.md[R77-80]

+   * In Playwright, uncaught exceptions propagate to end the session, which causes problems when
+     something unrelated to the test's intent goes wrong.
+   * Selenium is more lenient and only logs the error to the console.
+

Evidence

Ruby currently invokes handler blocks without rescue/logging, while Python explicitly catches and
logs exceptions, so the ADR’s statement is not uniformly true across Selenium bindings today.

docs/decisions/network-handler-behavior.md[77-80]
rb/lib/selenium/webdriver/common/network.rb[89-96]
py/private/_network_handlers.py[746-756]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The ADR text implies a universal Selenium behavior (“only logs the error to the console”) for handler exceptions, but current bindings differ.

### Issue Context
Ruby’s handler dispatch yields directly to user code without rescue; Python catches/logs and continues.

### Fix Focus Areas
- docs/decisions/network-handler-behavior.md[72-80]

### What to change
- Rephrase the bullet to one of:
 - “In some Selenium bindings (e.g., Python), uncaught handler exceptions are logged and processing continues…”
 - or “Proposed: Selenium should log uncaught handler exceptions and continue processing…”
- If desired, add a short note indicating current binding differences to avoid readers assuming uniform behavior today.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

Code review by qodo was updated up to the latest commit 18895d1

[titusfortner]

Actually, maybe the right answer here is to make this user-toggled with the default being to raise. I didn't consider this when suggesting alternatives below. Perhaps we do want to error-by-default but provide a global escape hatch while things are still in draft.

[AutomatedTester]

I feel fail fast should always be the right approach for people

[titusfortner]

First, to clarify, Python as written does the error behavior in this ADR (except for it doesn't discard staged changes).

As for fast-fail, that's correct for synchronous code evaluating the behavior. In network handlers, the failures will come from things the author didn't write like third-party scripts, analytics beacons, favicon retries, an auth challenge from an unrelated CDN. We don't want to necessarily couple the test's pass/fail to the open internet via a still-under-development BiDi protocol.

The reason I don't love this proposal as written is because when a user wants to mutate a specific thing as the intent of the test, silently ignoring it is a real problem, but I also don't want to de facto require users to always wrap everything in a try/catch which seems like a bad experience.

Honestly the desired default behavior is going to be different if a user wants to observe vs mutate, which might be a good reason to support the differentiation earlier. The alternative would be an error handling toggle for propagate vs log