Repository navigation
Conversation
network.continueWithAuth can reject when the request was already canceled or the page closed while the auth challenge was outstanding. The result was discarded with void, so the rejection escaped as an unhandled rejection and could terminate the embedding process. Await the command and tolerate the failure the same way _continue does, logging it on the error channel instead.
Add a case asserting the rejection is reported on the error channel and share the microtask drain helper between the cases.
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: APPROVE — ready for the operator to submit; no blocking issues found.
I reviewed the two changed files (the BiDi authentication rejection handling and its four focused unit cases), traced the pre-fix base implementation at 079789a903c32abf7156d6cfa08cabbc56e49037, and checked the candidate's upstream-policy, prior-art, boundary, and before/after test evidence. The base code's discarded continueWithAuth promises at packages/puppeteer-core/src/bidi/HTTPRequest.ts:318-329 can reject without an owner; the changed await … .catch paths at packages/puppeteer-core/src/bidi/HTTPRequest.ts:318-341 handle and debug-log both credential and cancel branches. The regression cases cover each rejecting branch, enabled/disabled logging, and the successive-authentication control. The CDP analogue likewise catches its command rejection. The claimed test, Prettier, and ESLint evidence is documented; this fork branch has no GitHub checks reported, and I did not run a local suite.
What's good: the patch is narrowly scoped, matches the existing CDP error-tolerance convention, includes a control that passes on both base and fixed arms, and the facts sheet accurately separates this defect from the adjacent browser-side proxy-auth issue. I found no competing upstream PR in the re-run prior-art search, no prohibited AI commit attribution, and no policy block. The operator should complete the documented CLA prerequisite before submitting upstream.
Notes for the operator:
- Upstream's PR-description/template requirements remain submission-time formatting work; the fork facts sheet has captured the relevant answers and evidence.
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: No blocking correctness issues found; the fix is sound and the test coverage discriminates for the cases it claims to cover. One scope question worth the operator's attention before submission (below), not a defect.
What I checked
- Full diff (
packages/puppeteer-core/src/bidi/HTTPRequest.ts+ newHTTPRequest.test.ts, 181/12 lines). bidi/core/Request.ts:102-111,317-330— confirmednetwork.authRequired→this.emit('authenticate', undefined)is fire-and-forget (common/EventEmitter.ts:136-142,emit()is synchronous, return value discarded), andcontinueWithAuthawaits#session.send('network.continueWithAuth', …), which can reject.cdp/NetworkManager.ts:435-442(#onAuthRequired) — confirmed it already doesvoid client.send(...).catch(err => this.#logger?.(DEBUG_PREFIXES.error)?.(err)), the CDP analogue the PR cites.api/HTTPRequest.ts:736-752(handleError) — confirmed it re-throws onInvalid header/Unsafe header/Expected "header"/invalid argument, and is used atbidi/HTTPRequest.ts:238in_continue, where the caller (interceptResolutionHandled→ user-facing promise chain) is awaited by callers, so a re-throw there is safe. In#handleAuthentication, nothing awaits the listener itself (emitdiscards the promise), so a re-throw fromhandleErrorwould reproduce exactly the unhandled-rejection bug this PR fixes. The PR's stated reason for not reusinghandleError()here checks out.git blame/history:eb874a58e("fix: logger calling causing crashes", puppeteer#15338) introduced the#loggerfield and constructor-injection pattern this PR relies on (this.#logger?.(DEBUG_PREFIXES.error)?.(error)), confirming the double optional-chain in rows 11-13 is the established idiom, not new.- No competing open PR touches
bidi/HTTPRequest.ts(checked open PR list by title/path; only puppeteer#15446, unrelatedHTTPResponse.asFetchResponse). - Test file placement/shape matches siblings
bidi/HTTPResponse.test.tsandbidi/Connection.test.ts(same license header,node:test+expect, cast mock objects, added the same week pergit log).
Findings
None rise to blocking. Two observations for the record:
-
Info — unconditional catch swallows non-auth-retryable errors too (
HTTPRequest.ts:327-332,338-340). The PR's own Boundaries row 8 already flags this as deliberate, matching the CDP twin. I traced it independently and agree:TargetCloseError extends ProtocolError(common/Errors.ts:91), and there's no code path where a different, non-tolerable error class reaches this catch that the CDP sibling doesn't already tolerate the same way. Not a defect, just worth the operator knowing this is an intentional divergence fromhandleError's selective re-throw, not an oversight — a maintainer might ask for a one-line comment distinguishing "we always tolerate here, unlike_continue," beyond what's already there. -
Info — no assertion that
interception.handledstate is unaffected. Unlike_abort/_respond/_continue(HTTPRequest.ts:236-247),#handleAuthenticationnever touchesthis.interception.handled, before or after this diff. That's correct (auth isn't a terminal interception outcome), but it means the new tests don't need to and don't assert on it — consistent, not a gap.
Boundaries re-derivation (spot check against the PR's own table)
I rebuilt the predicate list from the diff independently rather than trusting the PR body's table, focusing on the two rows most likely to hide a real gap:
- Row 14 equivalent (await replacing
void,HTTPRequest.ts:318,334): reproduced independently —emit()is synchronous and its return is discarded byRequest.ts:111, so awaiting inside the listener changes nothing observable to the emitter; the promise chain that used to be dropped is now attached to a.catch. Confirmed by readingEventEmitter.ts:136-142myself, not from the body. - Row 15 (
#authenticationHandled = trueat line 317, set before theawaitat line 318): reproduced — sinceemitnever awaits the handler, two synchronousauthenticateemissions in the same tick both run to the#authenticationHandledcheck before either handler'sawaityields, but the flag write happens synchronously before the firstawait, so the second emission's synchronous check already seestrue. No re-entrancy window. Test 4 ('should provide credentials only once and cancel afterwards') assertscalls[0].action === 'provideCredentials'andcalls[1].action === 'cancel', which is a real discriminator — it fails under a hypothetical regression that moved the flag write after the await. - The 4th test (control) is a real control: it is the only case that runs the
elsebranch with a successfulcontinueWithAuth, and it's the only test asserting the un-degraded two-call happy path, so it would fail if theawait/.catchwrapping broke normal resolution instead of just fixing the rejection path.
What's good
- Root-cause fix is minimal and scoped: only the two
continueWithAuthcall sites in#handleAuthentication, mirroring the exact pattern already proven safe in the CDP counterpart. - Test file is new but placed correctly per
docs/contributing.md(co-located with the class), matches sibling test shape/license header, and the "drain twosetImmediateturns" technique for detecting Node's unhandled-rejection check is correct and non-flaky as written. - Prior-art and in-repo-precedent research (CDP twin,
handleErrorre-throw semantics, the#loggerrefactor commit) is accurate — I verified each citation from source rather than trusting the summary.
SECOND READ: READY
Adds coverage for the cancel arm's error logging, non-Error rejection values, an unavailable logger, a second challenge emitted before the first settles, and empty credentials.
VerificationAdversarial verification by a fresh run. Nothing below is taken from the PR body — the boundary Head verified: Result: everything holds. The fix is real, the original 4 tests discriminate, and the 5 tests I EnvironmentAlpine/musl, Node v24.19.0. No Chrome in the image, so the e2e lane ( 1. The touched test file, fixed arm — 9/9$ node --test --test-reporter=spec "lib/puppeteer/bidi/HTTPRequest.test.js"
▶ BidiHTTPRequest
✔ should not reject when continueWithAuth fails while providing credentials (21.553605ms)
✔ should not reject when continueWithAuth fails while canceling (3.508004ms)
✔ should log the error when continueWithAuth fails (12.62757ms)
✔ should log the error when canceling the authentication fails (3.523322ms)
✔ should not reject when continueWithAuth fails with a non-Error value (1.87367ms)
✔ should not reject when continueWithAuth fails and logging is unavailable (1.809271ms)
✔ should provide credentials only once and cancel afterwards (1.489634ms)
✔ should cancel the second challenge emitted before the first one settles (3.341512ms)
✔ should provide empty credentials as-is (2.145168ms)
✔ BidiHTTPRequest (57.36096ms)
ℹ tests 9
ℹ suites 1
ℹ pass 9
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 652.4969992. The same 9 tests against the BASE source — 3 pass / 6 failMethod: keep the new test file, $ grep -n "continueWithAuth" lib/puppeteer/bidi/HTTPRequest.js | head -5
214: void this.#request.continueWithAuth({
224: void this.#request.continueWithAuth({$ node --test --test-reporter=spec "lib/puppeteer/bidi/HTTPRequest.test.js"
▶ BidiHTTPRequest
✖ should not reject when continueWithAuth fails while providing credentials (15.198052ms)
✖ should not reject when continueWithAuth fails while canceling (6.950113ms)
✖ should log the error when continueWithAuth fails (2.978594ms)
✖ should log the error when canceling the authentication fails (3.6457ms)
✖ should not reject when continueWithAuth fails with a non-Error value (2.118377ms)
✖ should not reject when continueWithAuth fails and logging is unavailable (1.486998ms)
✔ should provide credentials only once and cancel afterwards (13.728697ms)
✔ should cancel the second challenge emitted before the first one settles (1.872298ms)
✔ should provide empty credentials as-is (2.572035ms)
✖ BidiHTTPRequest (55.71258ms)
ℹ tests 9
ℹ suites 1
ℹ pass 3
ℹ fail 6
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 622.468115
✖ failing tests:
test at lib/puppeteer/bidi/HTTPRequest.test.js:73:5
✖ should not reject when continueWithAuth fails while providing credentials (15.198052ms)
ProtocolError: No such request with the given id
at TestContext.<anonymous> (file:///.../lib/puppeteer/bidi/HTTPRequest.test.js:75:45)
test at lib/puppeteer/bidi/HTTPRequest.test.js:92:5
✖ should not reject when continueWithAuth fails while canceling (6.950113ms)
ProtocolError: No such request with the given id
at TestContext.<anonymous> (file:///.../lib/puppeteer/bidi/HTTPRequest.test.js:94:45)
test at lib/puppeteer/bidi/HTTPRequest.test.js:111:5
✖ should log the error when continueWithAuth fails (2.978594ms)
ProtocolError: No such request with the given id
at TestContext.<anonymous> (file:///.../lib/puppeteer/bidi/HTTPRequest.test.js:118:23)
test at lib/puppeteer/bidi/HTTPRequest.test.js:133:5
✖ should log the error when canceling the authentication fails (3.6457ms)
ProtocolError: No such request with the given id
at TestContext.<anonymous> (file:///.../lib/puppeteer/bidi/HTTPRequest.test.js:133:23)
test at lib/puppeteer/bidi/HTTPRequest.test.js:143:5
✖ should not reject when continueWithAuth fails with a non-Error value (2.118377ms)
'No such request with the given id'Note the shape of the base failures: the tests do not fail on an assertion, they fail because the The 3 passing tests are the controls and are supposed to be green on both arms: they pin the 3. Boundary ledger, re-derived from the diffI rebuilt this from
Two rows the PR body's ledger did not have and that I consider genuine gaps worth closing — Row 6 is the only unreachable row and it is pre-existing code the diff does not touch. 4. Behaviour outside the stated bugRead the whole diff for side effects. One real behaviour change beyond "rejections are caught": I also checked whether 5. Package unit suite$ node --test --test-reporter=spec "lib/**/*.test.js"
ℹ tests 162
ℹ pass 153
ℹ fail 9All 9 failures are in 7 files that fail identically on clean base: i.e. a missing 6. Toolchain$ node node_modules/typescript/bin/tsc -b packages/puppeteer-core/tsconfig.json
$ grep -c "error TS" tsc.txt
1010 errors, and the same 10 on the base arm — pre-existing, from the unbuilt $ node node_modules/prettier/bin/prettier.cjs --check packages/puppeteer-core/src/bidi/HTTPRequest.test.ts
Checking formatting...
All matched files use Prettier code style!
$ node node_modules/eslint/bin/eslint.js --no-warn-ignored packages/puppeteer-core/src/bidi/HTTPRequest.test.ts
$ echo $?
0Both are what Verdict
|
Verification addendum — fork CIActions ARE enabled on this fork (contrary to an earlier note), and upstream's own workflows ran on The single failure is ##[error]No release type found in pull request title "[oss-candidate] fix(webdriver): handle continueWithAuth rejections".
Add a prefix to indicate what kind of release this pull request corresponds to.
For reference, see https://www.conventionalcommits.org/Upstream runs Operator action: the upstream PR title must be exactly with no bracket prefix. The two commit subjects on this branch already comply The remaining 23 checks are the e2e browser lanes; they were still pending and were not waited on. |
askalf
added
ready-for-operator
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: request changes — the facts sheet no longer provides complete, current-head evidence for the tests added in this head update.
Blocking findings
P1 — packages/puppeteer-core/src/bidi/HTTPRequest.test.ts:146-173 — current-head test evidence is incomplete
The live diff adds these regression cases:
it('should log the error when canceling the authentication fails', async () => {
it('should not reject when continueWithAuth fails with a non-Error value', async () => {
it('should not reject when continueWithAuth fails and logging is unavailable', async () => {
However, the candidate's ## Test evidence and base/fixed console transcript still describe only four cases and do not show the before/after result for these newly added cases. Its ## Verification method also says fork CI reported no checks, while this head has CI results. This makes the evidence sheet inaccurate for the exact artifact the operator would submit upstream. In particular, the new non-Error and absent-logger cases are important: the catch handler itself must not create a second escaped rejection.
Please update the facts sheet for this head with the full current test inventory and verbatim base/fixed output that covers every added case, then reconcile the current CI evidence (including the title-only required-check failure and the unrelated Firefox failure). The existing evidence-only rework path can be used because this correction is documentation/evidence only.
## Test evidence
Update the table and console transcripts from the current head so they include all nine tests, identify controls versus fix-sensitive tests, and report the current `gh pr checks` outcome with the reason each non-green job does or does not concern this change.
What's good
I traced the changed handler and the base version: both original bare void this.#request.continueWithAuth(...) calls could discard a rejection, while the new unconditional catches cover both credential and cancel paths. The added tests cover the meaningful edge cases, and the current CI has passing Puppeteer Unit and Chrome BiDi jobs. The Firefox failure log is in unrelated Page/Firefox protocol functionality rather than this hunk.
I reviewed the two changed files, the upstream-base authentication path, candidate policy and commit hygiene, current checks, and re-ran prior-art searches. I did not run the test suite locally.
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: not ready to submit upstream as-is — two of the newly added tests don't discriminate the fix (see Break-it pass below). The production fix itself is correct and well-precedented.
Independent trace of the bug
Confirmed from source at head a7f8c548 (not from the PR body): packages/puppeteer-core/src/bidi/HTTPRequest.ts:317-341 — #handleAuthentication is registered via this.#request.on('authenticate', this.#handleAuthentication) (line 125), and this codebase's EventEmitter.emit() (common/EventEmitter.ts:136-142) is synchronous and does not await the handler's return value. So switching void this.#request.continueWithAuth(...) to await this.#request.continueWithAuth(...).catch(...) inside #handleAuthentication changes nothing observable to emit()'s caller — only a rejection now has a handler. That matches the PR's own "why this is minimal" argument, and I verified it against the emitter source rather than taking the claim at face value.
The CDP-twin precedent is real: cdp/NetworkManager.ts:435-442 does .catch(err => { this.#logger?.(DEBUG_PREFIXES.error)?.(err); }) on the equivalent Fetch.continueWithAuth call — same shape as this PR's fix.
Break-it pass — Boundaries ledger (rebuilt from the diff, not the PR body)
| # | Guard/expression | file:line | Values | Test coverage |
|---|---|---|---|---|
| 1 | this.#logger?.(DEBUG_PREFIXES.error)?.(error) — provideCredentials catch |
HTTPRequest.ts:331 |
logger undefined / channel disabled / channel enabled | All three covered: "logging is unavailable" (undefined), "…while providing credentials" (disabled, default no-op logger), "should log the error when continueWithAuth fails" (enabled) |
| 2 | this.#logger?.(DEBUG_PREFIXES.error)?.(error) — cancel catch |
HTTPRequest.ts:339 |
logger undefined / channel disabled / channel enabled | Only disabled and enabled are covered ("…while canceling", "…canceling the authentication fails"). The logger field itself being undefined is exercised only for the credentials branch, not the cancel branch. Low severity — #logger is a required (non-optional) constructor param in production, so this state is defensive-only, matching the same pattern already in handleError. Info, not blocking. |
| 3 | continueWithAuthError !== undefined (test harness) |
HTTPRequest.test.ts:26 |
ProtocolError / string / undefined | Not a production boundary — internal to the fake, skip |
Break-it pass — per-test vacuity check (mandatory)
Two of the five tests added in the final commit (a7f8c548, "cover continueWithAuth edge cases") never set fakeRequest.continueWithAuthError, so continueWithAuth resolves without throwing in every arm:
HTTPRequest.test.ts:223should cancel the second challenge emitted before the first one settles— assertsfakeRequest.callssequence only.FakeRequest.continueWithAuthpushes intocallssynchronously on invocation regardless of whether the call site saysvoid x(...)(base) orawait x(...).catch(...)(fixed) — there's noawaitbefore the push insidecontinueWithAuthitself. I traced this: the assertion holds identically onorigin/main(pre-fix) and on this PR's head. It exercises the pre-existing#authenticationHandledlatch, which this diff does not touch.HTTPRequest.test.ts:235should provide empty credentials as-is— same reasoning: no error is ever thrown, and the credentials-object construction (type: 'password', username: credentials.username, password: credentials.password) is untouched code outside this diff's hunk. Passes identically before and after the fix.
Unlike should provide credentials only once and cancel afterwards (HTTPRequest.test.ts:210), which the PR body's own table (written for the earlier commit) explicitly labels CONTROL, these two are not labeled and aren't mentioned anywhere in the body — the test table is stale (it only accounts for the original 4-test commit ccb6fffd, not the 5 tests added afterward). A maintainer who diffs the test table against the actual test file will notice two assertions that can't fail whether or not the fix is applied and weren't flagged as intentional controls.
Suggested fix:
Either (a) drop the two tests since they duplicate coverage already implied by the
pre-existing behavior and are outside this bug's scope, or (b) keep them but label
them explicitly as controls in a comment, the way the PR body already does for the
"only once and cancel afterwards" case, and update the body's test table to list all
9 cases with pass/fail-on-base status for each.
The four tests actually claimed to pin the bug (providing credentials, while canceling, log the error when continueWithAuth fails, and by extension canceling the authentication fails / non-Error value / logging is unavailable added later) do genuinely discriminate: I traced that on origin/main (void, no catch), each of these sets continueWithAuthError to a truthy value, causing continueWithAuth to throw with nothing attached to the discarded promise — an unhandled rejection under Node's --unhandled-rejections=throw default. On this PR's head, the .catch() absorbs it. That part of the fix and its test coverage is sound.
Maintainer's-eye read
- Idiom: matches the codebase's established logger-refactor idiom from upstream commit
eb874a58("fix: logger calling causing crashes", 2026-08-13), which made#loggera required field onBidiHTTPRequestand standardizedlogger?.(DEBUG_PREFIXES.error)?.(error)as the tolerate-and-log pattern (seeapi/HTTPRequest.ts:748-751'shandleError, and the CDP twin cited above). This PR's shape is exactly that idiom, not an invented one. - Test shape: sibling unit tests in the same directory (
HTTPResponse.test.ts) use the samenode:test+expect+ license-header style; this PR's new file matches. - Scope: the diff is confined to
HTTPRequest.ts's#handleAuthenticationand its test file — appropriately narrow for a one-bug fix. The PR body's explicit disclaimer that this does not close issue puppeteer#14499 is honest and checks out against that issue's text (a Chrome-side gap, not fixable here). - Changelog/issue-first: I did not find an open upstream issue specifically describing this unhandled-rejection crash (searched
continueWithAuthandunhandledRejectioninpuppeteer/puppeteerissues/PRs); closest hits (puppeteer#14151, puppeteer#14152, puppeteer#14167, puppeteer#14499) are different bugs already correctly distinguished in the PR body. No existing PR touches this exact code path. This is a fine shape for a small, self-contained fix PR without a pre-filed issue. - Title:
fix(webdriver): handle continueWithAuth rejectionsmatches thetype(scope): descriptionconvention seen in recent merged titles (fix(webdriver): emit response event...,fix: logger calling causing crashes).
CI
gh pr checks shows two failures, both unrelated to the diff's correctness:
[Required] Validate PR titlefails only because of the[oss-candidate]prefix (semantic-PR-title bot) — a fork artifact, not a defect; the operator's real upstream title won't carry that prefix.firefox-headful (2-4)fails withsession not created: Maximum number of active sessions, severalunknown command/timeout errors in unrelated suites (userAgentClientHints,emulation.setScriptingEnabled,target.test,requestinterception-experimental) — none referenceHTTPRequest.ts,authenticate, orcontinueWithAuth. I read the job log directly; this looks like pre-existing shard flakiness, not something this diff caused.
What's good
The core fix is minimal, correctly reasoned from the emitter's synchronous-emit semantics, and matches an established in-repo idiom precisely (not just "similar," the same three-line pattern). The four tests that do pin the bug are correctly constructed and I verified by trace (not just PR-body claim) that they fail on base and pass on the fix.
SECOND READ: NOT READY — two new tests (HTTPRequest.test.ts:223 "should cancel the second challenge emitted before the first one settles" and HTTPRequest.test.ts:235 "should provide empty credentials as-is") pass identically with or without the fix and aren't labeled as controls or mentioned in the PR body's (now-stale) test table.
Three of the cases pass with and without the fix on purpose. Say so next to each one so the intent is not mistaken for a non-discriminating assertion.
Rework — round 2Both blocking findings were reproduced by execution here before anything was changed; neither was taken on faith. Head Second Read P1 — non-discriminating tests not labeled as controls — CONFIRMED, and it was three, not twoReviewer identified Base arm — $ node --test --test-reporter=spec "lib/puppeteer/bidi/HTTPRequest.test.js"
▶ BidiHTTPRequest
✖ should not reject when continueWithAuth fails while providing credentials (12.355174ms)
✖ should not reject when continueWithAuth fails while canceling (5.835372ms)
✖ should log the error when continueWithAuth fails (2.805671ms)
✖ should log the error when canceling the authentication fails (4.002729ms)
✖ should not reject when continueWithAuth fails with a non-Error value (2.857079ms)
✖ should not reject when continueWithAuth fails and logging is unavailable (2.026797ms)
✔ should provide credentials only once and cancel afterwards (13.933473ms)
✔ should cancel the second challenge emitted before the first one settles (5.322705ms)
✔ should provide empty credentials as-is (3.638238ms)
✖ BidiHTTPRequest (58.44501ms)
ℹ tests 9
ℹ pass 3
ℹ fail 6Fixed arm at $ node --test --test-reporter=spec "lib/puppeteer/bidi/HTTPRequest.test.js"
▶ BidiHTTPRequest
✔ should not reject when continueWithAuth fails while providing credentials (11.172033ms)
✔ should not reject when continueWithAuth fails while canceling (2.252277ms)
✔ should log the error when continueWithAuth fails (3.480574ms)
✔ should log the error when canceling the authentication fails (2.131482ms)
✔ should not reject when continueWithAuth fails with a non-Error value (1.10829ms)
✔ should not reject when continueWithAuth fails and logging is unavailable (1.437344ms)
✔ should provide credentials only once and cancel afterwards (1.113381ms)
✔ should cancel the second challenge emitted before the first one settles (1.871779ms)
✔ should provide empty credentials as-is (1.821883ms)
✔ BidiHTTPRequest (29.996665ms)
ℹ tests 9
ℹ pass 9
ℹ fail 0So exactly three tests pass on both arms, and they are exactly the three the reviewer pointed at plus the one already labeled. Took option (b), keep and label — commit
The body's test table now lists all nine with per-case base/fixed status and a Role column that says which are controls. Redline P1 — stale facts sheet — CONFIRMED, rewrittenThe body's table and console transcripts described the 4-case commit One thing worth calling out in the base output, because it reads like a broken test if you skim it: the base failures are not assertion failures. The reported reason is the CI reconciliation
##[error]No release type found in pull request title "[oss-candidate] fix(webdriver): handle continueWithAuth rejections".
Green at CI is running again at Unchanged
|
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: ready for the operator to submit; no blocking issues found.
I re-read the live-head diff, the rewritten facts sheet and round-2 rework comment, traced the base implementation at 079789a903c32abf7156d6cfa08cabbc56e49037, and checked the upstream for open continueWithAuth PRs (none). The new await … .catch covers both credential and cancel paths, and the test suite now distinguishes the six regression cases from the three explicitly documented controls. The boundaries ledger also accounts for the changed catch paths, unavailable logger, re-entrancy, and the synchronous-throw condition. Fork CI is largely green; one Chrome headless shard remains pending, while the title validation failure is caused by the fork-only [oss-candidate] prefix rather than the source change.
What's good: the minimal source fix is backed by focused colocated tests that fail on base and pass at this head, including both logging arms and the logger-unavailable path. The PR body now accurately records all nine cases and their base/fixed results.
Notes for the operator:
- Use the documented upstream title
fix(webdriver): handle continueWithAuth rejections(without the fork-only[oss-candidate]prefix), which satisfies the repository's conventional-title check. - Complete the Google CLA with the submitting account before opening upstream.
- The remaining
chrome-headless (2-2)check was pending when reviewed; verify it completes successfully before submission.
CI addendum — full run at
|
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: No blocking defects found; this is a sound, well-tested, upstream-idiom-matching fix at head 46789dc9aee9f0ffe35a885f7fbd274dc2898d37.
What I checked
- Full diff (
packages/puppeteer-core/src/bidi/HTTPRequest.ts, newHTTPRequest.test.ts, 279/– lines). - Production source at head via the GitHub contents API (
HTTPRequest.ts,core/Request.ts,cdp/NetworkManager.ts,common/Debug.ts,api/HTTPRequest.ts,bidi/Frame.ts). - Upstream recent history on this file (
gh api repos/puppeteer/puppeteer/commits?path=...) and theeb874a58logger refactor that introduced the non-optional#logger: Loggerfield this PR builds on. gh pr checks 1at this head.- Related open/closed upstream issues (
#14499,#14151,#14152,#14167) for prior art / scope honesty.
Correctness
#handleAuthentication (HTTPRequest.ts:311-341) changes void this.#request.continueWithAuth(...) to await this.#request.continueWithAuth(...).catch(error => { this.#logger?.(DEBUG_PREFIXES.error)?.(error); }), in both the provideCredentials and cancel branches. Request.continueWithAuth (bidi/core/Request.ts) rejects on session.send('network.continueWithAuth', ...) failure, e.g. ProtocolError('No such request with the given id') when the request is already gone. A void-discarded rejection is an unhandled rejection that kills the embedding Node process (default since Node 15) — I traced this end-to-end and it's real, not a static-analysis artifact.
The fix is correct and matches the already-shipped CDP twin almost byte-for-byte (cdp/NetworkManager.ts#onAuthRequired: .catch(err => { this.#logger?.(DEBUG_PREFIXES.error)?.(err); })), which is strong prior art for the idiom used here.
I checked whether making #handleAuthentication block on await changes behavior for its caller: it's registered via this.#request.on('authenticate', this.#handleAuthentication) and EventEmitter.emit (common/EventEmitter.ts:136-142) calls listeners synchronously without awaiting their return value, so no new blocking/timing regression is introduced by adding await.
Boundaries ledger (rebuilt from the diff, not the PR body)
| Row | Input | Fixed-code behavior | Pinned by |
|---|---|---|---|
continueWithAuthError (test double) undefined vs. set |
undefined | resolves normally | (implicit, every non-error test) |
same, set to Error instance |
ProtocolError(...) |
caught, logged, no reject | T1–T4 |
same, set to non-Error value |
plain string | caught, logged, no reject | T5 ("non-Error value") |
this.#logger itself |
undefined |
outer ?. short-circuits, no throw |
T6 ("logging is unavailable") |
this.#logger(prefix) return |
undefined (default test logger () => undefined) |
inner ?. short-circuits, no throw |
T1–T3, T5, T6 (all use the default logger) |
credentials truthiness |
{username:'', password:''} (falsy strings, truthy object) |
still provideCredentials, not cancel |
T9 (control) |
re-entrant authenticate emit before first settles |
two synchronous emits | second is cancel (flag set pre-await) |
T8 (control) |
Every row I could construct from the diff is exercised by an existing test, including both legs of the double optional chaining (this.#logger?.(...)?.(...)) — the outer-undefined case (T6) and the inner-undefined-return case (all others, via the default () => undefined logger). I did not find an uncovered reachable boundary.
Assertion check per test (base vs. fixed arm)
I independently traced, rather than trusting the PR body's console transcript: on origin/main's HTTPRequest.ts (still void-only), continueWithAuth rejecting is never awaited or caught, so T1–T6 (all of which assert rejections.toHaveLength(0) or an error being logged) fail on base and pass on the fix — these discriminate. T7–T9 are correctly labeled controls: none of them depend on the await/.catch change, so they pass on both arms; I did not find a labeled control that's secretly discriminating, nor an unlabeled test that's actually vacuous.
CI on this head (gh pr checks 1 --repo askalf/puppeteer) is green across the required lanes, including chrome-bidi (1-2)/(2-2) (the real-Chrome BiDi lane) and Puppeteer Unit tests. The only failure is [Required] Validate PR title, which is structural to this fork's mandated [oss-candidate] title prefix breaking amannn/action-semantic-pull-request's conventional-commit parse — not a defect in this PR, and it clears once the prefix is stripped for the actual upstream title.
Reuse / simplification (non-blocking)
HTTPRequest.ts:326-332 and :335-340 duplicate the same three-line "log and swallow" catch body twice. _continue two hunks above (HTTPRequest.ts:236-238) already calls a shared handleError(error, this.#logger) helper for the same "tolerate protocol errors from an already-canceled/closed request" case (api/HTTPRequest.ts:736-751). handleError's only special case (re-throwing on header-validation-message substrings) can't match a continueWithAuth rejection, so routing through it here would be behavior-preserving and remove the duplication. Not blocking — the CDP twin this PR mirrors (cdp/NetworkManager.ts#onAuthRequired) also inlines its own catch rather than using handleError, so the duplication matches an existing (if imperfect) local idiom.
Missing tests
None — auth-rejection coverage is symmetric across both branches (provideCredentials and cancel), both Error and non-Error rejection shapes, and both legs of the optional-chained logger call.
Upstream fit (maintainer's-eye read)
- Title
fix(webdriver): handle continueWithAuth rejectionsmatches the repo's establishedfix(webdriver): ...scope convention (e.g.fix(webdriver): emit response event on network.responseStarted,fix(webdriver): support interception after continueWithAuth). - No
.changesetdirectory exists in this repo, so no changelog-entry convention applies here (operator note, not a finding). - Test file style (
node:test+expect, license header, onedescribeper class, inline fakes rather than mocking libraries) matches siblingHTTPRequest.test.ts/HTTPResponse.test.ts/Connection.test.tsin the same directory exactly. - Related upstream issue
#14499("page.authenticate does not work over the webdriver bidi protocol") is open and adjacent but distinct; the PR body's own "Scope honesty" section already disclaims fixing it, which matches what I found in the code — this fix only addresses the unhandled-rejection crash, not#14499's broader BiDi-auth functionality gap.
I could independently confirm the underlying bug from the base code (not just from the PR's own transcript): bidi/core/Request.ts's continueWithAuth awaits a session.send that can reject, and the pre-PR HTTPRequest.ts discarded that promise with a bare void in both branches, with no other consumer attached — an unhandled rejection is the only possible outcome of that rejection path.
SECOND READ: READY
Adds the cancel-arm counterparts of the non-Error and missing-logger cases, a TargetCloseError case for the page-closed path, a both-arms-fail case and a control for credentials supplied after an earlier challenge was canceled.
VerificationIndependent adversarial pass over this branch by a fresh run, at head The production source is unchanged. What the ledger rebuild foundThe diff adds two independent
A/B, verbatimBase arm = ### BASE ARM
ℹ tests 14
ℹ suites 1
ℹ pass 4
ℹ fail 10
### FIXED ARM (f3280903)
ℹ tests 14
ℹ suites 1
ℹ pass 14
ℹ fail 0The 4 base-arm passers are exactly the 4 declared controls (T7, T8, T9, T14). Every One thing to read carefully: base-arm failures here are not assertion failures. Other checks at this head
One hole reported, not fixedA user-supplied Verified: the fix is minimal, the tests discriminate, the controls are labelled, and nothing outside |
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: request changes — the OSS-candidate facts sheet contains stale, contradictory reproduction evidence and is not ready for operator submission.
Blocking — facts sheet does not describe the reviewed head
packages/puppeteer-core/src/bidi/HTTPRequest.test.ts:245-284
it('should not reject when canceling fails with a non-Error value', async () => {
it('should not reject when canceling fails and logging is unavailable', async () => {
it('should not reject when the page closes while the challenge is outstanding', async () => {
it('should not reject when both arms fail in turn', async () => {
it('should provide credentials set after an earlier challenge was canceled (control)', async () => {
The live diff contains 14 tests, including these five verification additions, and the Summary/Test evidence sections correctly show a 14-test base/fixed comparison. However, ## Repro still says “Output below is the current 9-test file” and presents only the earlier 9-test base output. That is a false statement about the submitted artifact and leaves conflicting evidence in the operator-facing upstream description. An upstream maintainer cannot reliably determine which reproduction result corresponds to this PR head.
Suggested fix:
Replace the stale nine-test Repro transcript with the same 14-test base-arm command/output used for this head (or explicitly label the nine-test transcript as historical and link it to its old SHA). Ensure every test count and “current” claim in the facts sheet agrees with f3280903.
What's good: I traced the base implementation: both authentication branches discard the continueWithAuth promise with void, while the patch awaits and catches each branch. The new tests cover both catch sites, non-Error rejections, unavailable logging, closure, sequential failures, and the changed ordering/re-entrancy boundaries. The base implementation at 079789a903 reproduces the uncaught-promise shape by inspection, no duplicate upstream PR/issue was returned by my fresh searches, commit messages follow Conventional Commits, and the full relevant fork CI matrix is green apart from the documented fork-title-only semantic-title check.
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: No blocking defects found in the production diff; this round is test-comment-only over the already-reviewed source.
What I checked
- Diff at head
f3280903vs46789dc9(the head both seats previously reviewed):packages/puppeteer-core/src/bidi/HTTPRequest.ts— byte-identical, confirmed withgit diff 46789dc9 f3280903 -- packages/puppeteer-core/src/bidi/HTTPRequest.ts(empty). This round's delta is entirely inHTTPRequest.test.ts: labeling four already-present cases as(control)/Control:comments plus commentary — no new test bodies, no assertion changes. - Traced
#handleAuthentication(packages/puppeteer-core/src/bidi/HTTPRequest.ts:310-341) againstRequest.continueWithAuth(packages/puppeteer-core/src/bidi/core/Request.ts:317-334), whichawaitssession.send('network.continueWithAuth', …)and can reject withProtocolError. - Compared the fix's shape against the two existing tolerant call sites in the same class (
_continueatHTTPRequest.ts:236-239viahandleError,_abortatHTTPRequest.ts:244-247) and the CDP twincdp/NetworkManager.ts:435-442, which already doesvoid client.send(...).catch(err => this.#logger?.(DEBUG_PREFIXES.error)?.(err)). - Pulled upstream history on this file (
gh api repos/puppeteer/puppeteer/commits?path=...HTTPRequest.ts): commiteb874a58("fix: logger calling causing crashes (puppeteer#15338)") introduced the exactthis.#logger?.(prefix)?.(error)optional-chaining idiom this PR reuses, and is the same commit that threadedLoggerthroughBidiHTTPRequest.from/constructor — this PR'sDEBUG_PREFIXESimport and logger-call shape matches it exactly. - Confirmed
fix(webdriver): ...is an established upstream title prefix viasearch/issues(multiple merged examples), and that CHANGELOG.md is release-please-generated (not hand-edited in the reference commit), so no changelog line is expected here. - Ran
gh pr checks 1: all required checks green except[Required] Validate PR title, which fails solely because the fork's mandated[oss-candidate]prefix breaksamannn/action-semantic-pull-request's conventional-commit parse — structural to the fork's own labeling convention, not a defect in the change (the PR body already documents the suggested upstream title without the prefix).
Boundaries ledger (rebuilt from the diff, not the body)
The diff changes one predicate (credentials && !this.#authenticationHandled, unchanged from before) and adds two .catch sites. Rows:
| # | Input / condition | Fixed code behavior | Pinned by |
|---|---|---|---|
| 1 | continueWithAuth rejects with ProtocolError, credentials arm |
.catch logs via DEBUG_PREFIXES.error, no throw |
HTTPRequest.test.ts:100 "should not reject when continueWithAuth fails while providing credentials" |
| 2 | same, cancel arm | .catch logs, no throw |
HTTPRequest.test.ts:123 "...while canceling" |
| 3 | rejection value is a non-Error string, credentials arm |
logger still invoked with raw value; no throw | :187 "...non-Error value" |
| 4 | rejection value is TargetCloseError (page closed mid-challenge) |
tolerated | :290 "...page closes..." |
| 5 | logger === undefined (host passed no logger) |
this.#logger?.(...) short-circuits, no throw from the catch handler itself |
:199, :275 (credentials + cancel arms both covered) |
| 6 | cancel arm + non-Error rejection | tolerated | :262 |
| 7 | cancel arm + logger === undefined |
tolerated | :275 |
| 8 | both arms invoked in sequence (credentials then cancel), both reject | both tolerated independently, #authenticationHandled latch still set once |
:302 "...both arms fail in turn" |
| 9 | #authenticationHandled already true, second emit('authenticate') arrives before the first await resolves (re-entrancy) |
second call takes the cancel branch since the flag was set synchronously before the await |
:234 "...before the first one settles" (unaffected-by-fix control, correctly labeled) |
| 10 | credentials set only after an earlier challenge was already canceled (reverse order) |
second challenge correctly takes credentials arm | :325 "(control)" |
| 11 | empty-string credentials ({username: '', password: ''}) |
still treated as truthy credentials object, provided as-is, not coerced to cancel |
:248 |
All 11 reachable rows from this diff are covered and each points at a real, distinct assertion — I don't have a row the test suite misses. One adjacent row is not introduced or widened by this PR and is correctly left alone: this.#logger?.(DEBUG_PREFIXES.error)?.(error) itself would throw if a caller supplied a logger function that throws synchronously — but that's the pre-existing idiom shared by ~20 other call sites (including the CDP twin this PR mirrors), not something this diff changes the risk profile of. Not a finding against this PR.
Reuse / idiom fit
The fix reuses the exact tolerance pattern already established at three sibling call sites in the same file and its CDP counterpart, rather than inventing a new error-handling shape — this is the idiom a maintainer familiar with eb874a58/the "refactor logger" PR series would expect. No existing shared helper (e.g., handleError) was available for this exact call shape since handleError is _continue-specific (rethrows on certain messages); inlining the .catch here matches _abort's and the CDP twin's local-.catch style rather than _continue's, which is the correct choice given this call site has no analogous rethrow-on-specific-message requirement.
Test shape
HTTPRequest.test.ts follows the same node:test + expect + no-sinon style as the pre-existing sibling HTTPResponse.test.ts and cdp/HTTPRequest.test.ts in this repo (both confirmed via upstream history), so the new file's shape is not novel for this codebase.
SECOND READ: READY
ReworkRedline's blocking finding at Rather than re-label the old transcript as historical, I re-executed the whole A/B at this head so ### BASE ARM — 079789a903 src/bidi/HTTPRequest.ts, the 14-test file of this head in place
$ git checkout 079789a903 -- packages/puppeteer-core/src/bidi/HTTPRequest.ts
$ node node_modules/typescript/bin/tsc -b packages/puppeteer-core/tsconfig.json # 10 pre-existing errors
$ grep -n continueWithAuth packages/puppeteer-core/lib/puppeteer/bidi/HTTPRequest.js
214: void this.#request.continueWithAuth({
224: void this.#request.continueWithAuth({
$ cd packages/puppeteer-core && node --test --test-reporter=spec "lib/puppeteer/bidi/HTTPRequest.test.js"
▶ BidiHTTPRequest
✖ should not reject when continueWithAuth fails while providing credentials (17.535261ms)
✖ should not reject when continueWithAuth fails while canceling (5.082479ms)
✖ should log the error when continueWithAuth fails (3.297693ms)
✖ should log the error when canceling the authentication fails (2.089966ms)
✖ should not reject when continueWithAuth fails with a non-Error value (6.647503ms)
✖ should not reject when continueWithAuth fails and logging is unavailable (2.257759ms)
✔ should provide credentials only once and cancel afterwards (18.285624ms)
✔ should cancel the second challenge emitted before the first one settles (2.087792ms)
✔ should provide empty credentials as-is (3.63311ms)
✖ should not reject when canceling fails with a non-Error value (2.106447ms)
✖ should not reject when canceling fails and logging is unavailable (2.062806ms)
✖ should not reject when the page closes while the challenge is outstanding (2.094244ms)
✖ should not reject when both arms fail in turn (2.51471ms)
✔ should provide credentials set after an earlier challenge was canceled (control) (7.15957ms)
✖ BidiHTTPRequest (84.002101ms)
ℹ Error: Test "should not reject when both arms fail in turn" at lib/puppeteer/bidi/HTTPRequest.test.js:228:5 generated asynchronous activity after the test ended. This activity created the error "ProtocolError: No such request with the given id" and would have caused the test to fail, but instead triggered an unhandledRejection event.
ℹ tests 14
ℹ pass 4
ℹ fail 10
### FIXED ARM — fixed source restored, rebuilt, same file
$ cd packages/puppeteer-core && node --test --test-reporter=spec "lib/puppeteer/bidi/HTTPRequest.test.js"
▶ BidiHTTPRequest
✔ should not reject when continueWithAuth fails while providing credentials (28.105442ms)
✔ should not reject when continueWithAuth fails while canceling (3.198236ms)
✔ should log the error when continueWithAuth fails (6.611786ms)
✔ should log the error when canceling the authentication fails (3.154675ms)
✔ should not reject when continueWithAuth fails with a non-Error value (2.855566ms)
✔ should not reject when continueWithAuth fails and logging is unavailable (2.568931ms)
✔ should provide credentials only once and cancel afterwards (3.113759ms)
✔ should cancel the second challenge emitted before the first one settles (2.092161ms)
✔ should provide empty credentials as-is (2.653178ms)
✔ should not reject when canceling fails with a non-Error value (3.593034ms)
✔ should not reject when canceling fails and logging is unavailable (1.997223ms)
✔ should not reject when the page closes while the challenge is outstanding (2.400456ms)
✔ should not reject when both arms fail in turn (2.880452ms)
✔ should provide credentials set after an earlier challenge was canceled (control) (6.14279ms)
✔ BidiHTTPRequest (78.889878ms)
ℹ tests 14
ℹ pass 14
ℹ fail 0
$ git status --porcelain # after restoring the fixed source
$ git rev-parse HEAD
f3280903ca894bb262d75ad7e0c8c05614ebb8ce
$ grep -c "^ it(" packages/puppeteer-core/src/bidi/HTTPRequest.test.ts
14
$ grep -c "error TS" base-build.log ; grep -c "error TS" fixed-build.log
10
10Body changes, all reconciled to
I grepped the whole body for test counts and for "current"/"now" afterwards; the remaining "current" |
sprayberry-redline
left a comment
sprayberry-redline
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the GPT gating lane (gating review).
Verdict: ready for the operator to submit; no blocking issues found in this evidence-only re-review.
I re-read the live-head diff and the rewritten facts sheet at f3280903ca894bb262d75ad7e0c8c05614ebb8ce. The former stale ## Repro claim is corrected: it now explicitly identifies the complete 14-test file at this head and gives a matching base-arm transcript (4 pass / 10 fail) and fixed-arm transcript (14 pass / 0 fail). The boundaries ledger covers each independently reachable credentials and cancel .catch and logger chain, including the new cancel-arm cases. I also traced the base implementation at 079789a903, where both calls are bare void expressions, and independently repeated upstream prior-art searches; there is no open upstream PR for this defect.
The implementation change handles the two previously discarded protocol promises on both paths:
packages/puppeteer-core/src/bidi/HTTPRequest.ts:318-336:await this.#request.continueWithAuth(...)followed by.catch(error => { this.#logger?.(DEBUG_PREFIXES.error)?.(error); })packages/puppeteer-core/src/bidi/HTTPRequest.ts:337-341: the correspondingcancelcall has the same rejection handling.
These changes match the stated failure mode (a rejected BiDi command after a stale request) and the added tests discriminate on the base source. The fork CI matrix is green for the relevant unit and BiDi lanes. The sole failed title check is explained by the fork-only [oss-candidate] prefix and does not apply to the suggested upstream title.
Notes for the operator:
- Sign the Google CLA before upstream submission if the submitting account has not already done so.
- Use the body’s suggested conventional-commit upstream title without the fork-only
[oss-candidate]prefix.
What's good: this is a narrow fix, it catches each independently reachable rejection path, and the evidence-only rework reconciles the test counts and transcripts to the actual reviewed head.
sprayberry-secondread
left a comment
sprayberry-secondread
left a comment
There was a problem hiding this comment.
Automated review from the Sprayberry Labs fleet code reviewer.
Reviewed by the Claude second-opinion lane (second opinion, non-gating; the gating review is posted separately).
Verdict: This is an evidence-only rework at the same head I already reviewed (f3280903, 19:23:56Z). The production source is unchanged; only the PR body/facts sheet was rewritten to fix a stale claim (the ## Repro section previously said "current 9-test file" against a 14-test branch). I re-read the whole body against the live diff, the live test file, and upstream puppeteer/puppeteer source/history rather than trusting the prior pass's notes.
What I independently checked
- Diff is unchanged from my last read: 2 files,
HTTPRequest.ts(+23/-12) and newHTTPRequest.test.ts(354 lines, 14it(cases, confirmed bygrep -c "^ it("on the live blob atf3280903). - Every number the body now cites matches the live artefacts: 14 tests, 5 commits (
gh api .../pulls/1/commitsreturns exactlya8449423a8, ccb6fffd06, a7f8c5483b, 46789dc9ae, f3280903ca), no stray "9-test"/"four commits" strings survive (grep -icame back empty). - Read the actual base (
079789a903) and head (f3280903) copies ofbidi/HTTPRequest.ts,bidi/core/Request.ts,common/EventEmitter.ts, andapi/HTTPRequest.tsfrom GitHub directly (not the PR diff) to confirm the cited line numbers and behaviour (e.g.continueWithAuthisasync/no@inertIfDisposed,emit()is synchronous,handleError's re-throw conditions) are real, not asserted. - Ran
gh pr checks 1myself: 30 pass / 1 fail, the one failure is[Required] Validate PR title, which is exactly the fork's own[oss-candidate]prefix breakingamannn/action-semantic-pull-request's conventional-commit parse — confirmed by reading the job's error text. Not a defect in the diff. - Re-ran the prior-art searches myself (
gh search prs/issues --repo puppeteer/puppeteer "continueWithAuth","unhandledRejection","handleAuthentication",gh pr list --state open): identical result set to what the body reports. No open PR touchesbidi/HTTPRequest.ts.
Maintainer's-eye read (upstream idiom)
Pulled gh api repos/puppeteer/puppeteer/commits?path=packages/puppeteer-core/src/bidi/HTTPRequest.ts — the most recent history-relevant hit is puppeteer#15338 "fix: logger calling causing crashes" (merged 2026-08-13), which reworked the exact Logger plumbing this PR touches: it changed #logger from Logger | undefined/logger?: Logger to a required Logger parameter threaded through every constructor (BidiHTTPRequest, BidiFrame, BidiBrowser, CdpHTTPRequest, NetworkManager), replacing ad hoc (this.frame() as any).logger reads. That PR was itself closing two crash reports (#15336, #15337) about exactly this class of "unhandled rejection escapes past a bad error-handling path" bug. Two implications for the operator:
- The idiom this PR uses (
this.#logger?.(DEBUG_PREFIXES.error)?.(error)inside.catch) is already the codebase's current, post-puppeteer#15338 convention — confirmed by readingcdp/NetworkManager.ts:441andbidi/Browser.ts:268/398at this head, both unchanged by this diff and both doing the identical two-?.chain. The PR is not introducing a new pattern; it is applying the existing one to a call site puppeteer#15338 didn't touch. That is a point in this PR's favor for a maintainer — it completes a cleanup they already started, in the same idiom. - puppeteer#15338's own title/shape ("fix: X causing crashes", closing two linked issues, touching ~10 files") shows this maintainer team tolerates a "fix real crash, keep scope on the exact defect" PR — supports the operator's chosen scope (one bug, one file's
#handleAuthentication, deliberately not folding in the row-19 throwing-logger property or thehandleErrorreuse). - Test shape: compared against
cdp/HTTPRequest.test.ts(upstream, same directory family) —node:test+expect, plain cast mock objects ({} as CDPSession), constructor-injectedlogger: () => undefined. The newHTTPRequest.test.tsin this PR follows that shape closely (same import style, samedescribe/it, same logger-stub pattern). No idiom mismatch found.
Break-it pass — Boundaries ledger, rebuilt independently from the diff
I rebuilt the boundary set from the diff myself before reading the body's ## Boundaries table (22 rows). My independent list matches the body's rows 1–5 (credentials-present/absent × first/second/reverse challenge), 7–14 (resolve/reject × Error/non-Error/TargetCloseError × both arms), 15–19 (logger undefined/absent/throwing × both arms), 20–21 (await/emit synchronicity, re-entrancy), 22 (sync-throw-in-callee check). I did not find a reachable predicate, guard, or truthiness check added by this diff that the body's ledger misses. Row 19 (throwing logger re-creates the unhandled rejection inside the new .catch) is honestly flagged as measured-not-fixed and cross-checked against cdp/NetworkManager.ts:441 as a pre-existing, codebase-wide shape rather than something this diff introduces — I agree with that framing; guarding it here alone would diverge from ~20 sibling call sites, including the one puppeteer#15338 just touched without fixing this property either.
Test-assertion audit (per test, not per row)
Read all 14 tests in the live file. T7/T8/T9/T14 are labelled controls and are genuinely both-arms-green for reasons connected to the diff (T8 pins the latch-before-await ordering that the await could have broken; T14 pins the reverse-order interleaving). None of the 14 assertions is vacuous under any variant I could find — each of T1–T6, T10–T13 asserts either rejections.toHaveLength(0) after a real rejection is injected via continueWithAuthError, or a specific logged payload, and each traces to a line the diff actually changed (confirmed against the live source, not the body's claim).
Findings
None that block. No correctness bug found; the fix is a straightforward void → await + .catch change consistent with the file's other three call sites and the CDP twin, and the tests discriminate as claimed.
What's good
- The body-rewrite this round did what it needed to: every count I could grep for (tests, commits, CI numbers) now matches the live artefacts, and the fourth-pass A/B transcript is freshly re-derived at this head rather than inherited.
- The maintainer-idiom check turned up genuinely useful context (puppeteer#15338) that neither this body nor my prior read had surfaced — worth the operator citing it in the upstream PR description as evidence the logger-plumbing pattern is current convention, not this PR's own invention.
- Prior-art and CI claims both reproduced exactly under my own re-run.
SECOND READ: READY
|
Submitted upstream for review. |
Summary
BidiHTTPRequest.#handleAuthenticationfired bothnetwork.continueWithAuthcommands with a barevoid, discarding the returned promise.Request.continueWithAuth()(bidi/core/Request.ts:317) awaitssession.send('network.continueWithAuth', …), which rejects with aProtocolErrorwhen the request is already gone — canceled, page closed, or the challenge already answered._continue→handleError,_abort→.catch,_respond→.catch); the auth path was the only one that did not. The CDP twin (cdp/NetworkManager.ts:435-442) already does exactly what this patch does.awaitthe command and.catchit, logging onDEBUG_PREFIXES.error— the same tolerance ruleapi/HTTPRequest.ts:748-751documents ("In certain cases, protocol will return error if the request was already canceled or the page was closed. We should tolerate these errors.").Both arms above are from one A/B run at head
f3280903(2026-09-14, fourth pass); the same runproduced the Repro transcript, so every number in this sheet comes from one head.
The four that pass on both arms are the four deliberate controls (T7, T8, T9, T14 in
Test evidence); each is marked as a control in a comment above it in the test file. Every other
case fails on base and passes with the fix.
Read the base-arm failures carefully — they are not assertion failures. The escaped rejection is
attributed by
node:testto whichever test happens to be running, so base output showsProtocolError: No such request with the given id(aTargetCloseErrorfor the page-close case,and for the two non-
Errorcases the bare string) with a stack into the test file, plus lines ofthe form
Test "…" generated asynchronous activity after the test ended … triggered an unhandledRejection event. That is exactly the defect under test surfacing; it discriminatescorrectly. The full failure detail is in Repro.
Upstream
puppeteer/puppeteermain079789a903c32abf7156d6cfa08cabbc56e49037f3280903ca894bb262d75ad7e0c8c05614ebb8ce(production source byte-identical to46789dc9, which both reviewers read; the delta is test-only)packages/puppeteer-core/src/bidi/HTTPRequest.ts(#handleAuthentication, lines 310-342, 23 insertions / 12 deletions), new testpackages/puppeteer-core/src/bidi/HTTPRequest.test.ts(14 cases)Bug
Trigger. Any WebDriver BiDi session where
page.authenticate()has installed anauthRequiredinterception (
bidi/Page.ts:778) and the browser emitsnetwork.authRequiredfor a request that isno longer answerable by the time puppeteer replies — the request was canceled, the page/context was
closed while the challenge was outstanding, or the challenge was already answered by another path.
Request.continueWithAuth()then rejects withProtocolError(e.g.No such request with the given id).Wrong outcome.
#handleAuthenticationdiscarded that promise withvoid, so nothing wasattached to handle the rejection. Node emits
unhandledRejection, and since Node 15 the default--unhandled-rejections=throwturns that into a fatal uncaught exception: the embedder's processexits. The error is not surfaced through
page.on('error'), thePageEvent.RequestFailedpath, orany promise the caller holds, so from the user's side a proxy-auth navigation simply kills the
process with a stack pointing into puppeteer internals.
Blast radius. Every
protocol: 'webDriverBiDi'user who callspage.authenticate()— thedefault protocol for Firefox and an increasingly common choice for Chrome. The race is not exotic:
closing the page or aborting navigation while an auth challenge is in flight is enough, and the
larger the number of in-flight authenticated requests, the more likely one of them is stale when
puppeteer answers. The CDP path is unaffected because
cdp/NetworkManager.ts:440already catches.Scope honesty (important for the upstream description). This is not a fix for issue puppeteer#14499
("page.authenticate does not work over the webdriver bidi protocol"). That reporter demonstrated
with their own raw BiDi client that Chrome never sends
network.authRequiredfor proxychallenges — a browser-side gap that cannot be fixed in this repo. The puppeteer-side wiring
(
Page.ts:778→addIntercept([authRequired])→Browser.ts:107subscribenetwork→core/Request.ts:102→HTTPRequest.ts:129) was traced end to end and is correct. This PR fixes aseparate, independently provable defect found on that surface. Do not claim it closes puppeteer#14499.
Repro
The failure is a race against protocol state, so the repro is the unit test in this PR, driven
against the base build. It reproduces exactly the condition described above: the
authenticateevent fires and
continueWithAuthrejects.The transcript below is the complete 14-test file as it stands at this head (
f3280903) runagainst
079789a903'ssrc/bidi/HTTPRequest.ts— the same base arm as Summary, re-executed forthis head rather than carried over from an earlier one: 4 pass / 10 fail. The four passes are the
four declared controls (T7, T8, T9, T14 in Test evidence).
With the fix restored and rebuilt, the same command on the same file reports
ℹ tests 14 / ℹ pass 14 / ℹ fail 0— the full fixed-arm transcript is in Summary.Read the base failures carefully — they are not assertion failures. The reported reason is the
ProtocolErroritself (for the page-close case aTargetCloseError, and for the two non-Errorcases the bare string
'No such request with the given id'), with a stack pointing into the testfile. That is the symptom: the rejection escapes
#handleAuthenticationentirely, nothing inpuppeteer owns the promise, and
node:testattributes the escaped rejection to whichever testhappens to be running — the
generated asynchronous activity after the test ended … triggered an unhandledRejection eventline says so explicitly. In a real embedding application there is no testrunner to attribute it to; the process exits.
Fix
Net 23 insertions / 12 deletions in one source file.
Why this is minimal and correct.
#handleAuthenticationis already anasyncarrow assigned toan
EventEmitterlistener, so awaiting inside it changes nothing observable for callers (theemitter ignores the returned promise;
emitis synchronous percommon/EventEmitter.ts:136-142).The only behaviour change is that a rejection now has a handler. Logging on
DEBUG_PREFIXES.erroris the established convention for exactly this class of error in this codebase —
api/HTTPRequest.ts:736-752(handleError) andcdp/NetworkManager.ts:435-442both do it, and thelatter is the byte-for-byte CDP counterpart of this code path.
Alternatives rejected.
void … .catch(…), keeping thevoid. Equivalent for rejection safety, butawaitalso makesthe handler's completion observable to a future caller and matches
_continue/_abort/_respondin the same class, which all
await/return await. Chose consistency with the file.handleError(error, this.#logger)fromapi/HTTPRequest.ts. Rejected:handleErrorre-throws for
Invalid header/Unsafe header/invalid argumentmessages. Re-throwing fromthe auth handler would reintroduce the unhandled rejection for exactly the input class where the
BiDi spec says a bad
credentialsvalue producesinvalid argument. The interception path canre-throw because it runs inside a user-awaited
_continue; the auth path has no such owner.PageEvent.RequestFailedor a pageerrorevent. A behaviour change beyond the bug, andnot what the CDP path does. Out of scope for a one-bug PR.
#authenticationHandledon failure so a retry can re-provide credentials. Would changethe retry semantics and risk a credential loop against a server that keeps re-challenging. Not
needed to fix the crash; deliberately left alone.
Test evidence
New file
packages/puppeteer-core/src/bidi/HTTPRequest.test.ts, following the sibling unit tests inthe same directory (
HTTPResponse.test.ts,Connection.test.ts) —node:test+expect, plaincast mock objects, no browser required.
FakeRequestextends the realEventEmitterand throws areal
ProtocolError/TargetCloseError(or, for T5 and T10, a bare string) fromcontinueWithAuth.Fourteen cases — every test now on the branch is listed. "Base" is
origin/main'ssrc/bidi/HTTPRequest.ts(079789a903) with this same 14-test file present; "Fixed" is this PR'shead
f3280903. Both columns are transcribed verbatim from the two runs quoted in Summary.T10–T14 were added by an independent adversarial verification pass over this branch.
Erroris still swallowed and logged#loggeritselfundefined— the first?.. This is the branch that would re-create the unhandled rejection inside the catch handler if it threw.catchcallbacks; T5 only covered the credentials one?.on the second logger chainTargetCloseError(a distinct subclass), pinning that the catch really is type-agnostic. This was ledger row 9, previously argued but untestedOn the four controls. T7, T8, T9 and T14 pass identically with and without the fix, by design,
and each carries a comment in the test file saying so. They are not evidence that the bug exists;
they are evidence that the fix does not break what already worked. Each controls for a different
thing:
#authenticationHandledlatch),so "ten tests fail on base" cannot be explained by the patch simply breaking normal auth.
voidwithawait. If the latch were set after the await, a secondchallenge arriving before the first settled would provide credentials twice. It is set before
(line 317), and T8 pins that. The interleaving is the one this diff could plausibly have broken.
object. T9 pins that they are still forwarded as
provideCredentialsrather than collapsing tocancel.cancel,latch untouched), credentials are set afterwards, a second challenge arrives (→
provideCredentials). Because#authenticationHandledis only set on the credentials arm, acancel must not consume the one-shot latch. Nothing else on the branch exercises that direction.
A maintainer diffing the test table against the file will find fourteen cases here and fourteen in
the file (
grep -c "^ it(" packages/puppeteer-core/src/bidi/HTTPRequest.test.ts→14).Method for the two arms (no
git stashacross worktrees): keep the test file in place,cpthe fixed source aside,git checkout 079789a903 -- packages/puppeteer-core/src/bidi/HTTPRequest.ts,rebuild, run; then
cpthe fixed source back,git reset HEAD -- <src>, rebuild, run. The two armsdiffer only by
src/bidi/HTTPRequest.ts. After restoring,git diff --stat -- packages/puppeteer-core/src/bidi/HTTPRequest.tsis empty, i.e. the productionsource at this head is byte-identical to the head both reviewers read.
Tooling (the commands
docs/contributing.mdnames), all run at the current headf3280903:Package unit lane at this head:
All 7 failures are whole files that fail to import
lib/puppeteer/generated/injected.js(ERR_MODULE_NOT_FOUND) —bidi/ScreenRecording.test.js,cdp/Browser.test.js,cdp/Frame.test.js,common/BrowserConnector.test.js,node/ChromeLauncher.test.js,node/FirefoxLauncher.test.js,node/PuppeteerNode.test.js. Thatartifact comes from the
postinstall/build:toolsstep skipped bynpm install --ignore-scripts;the same 7 fail on clean base. None is
bidi/HTTPRequest.test.js. The fork's own CI runs the realPuppeteer Unit testsjob with a full build — see Verification method.Verification method
executed, in-container: Alpine/musl, Node v24.19.0,
npm install --ignore-scripts(no browserbinaries). Puppeteer's unit lane (
node --testagainstlib/) runs here without a browser; the e2elane (
test/src/network.test.ts, mocha-runner + real Chrome/Firefox + a proxy) does not — thereis no Chrome or Chromium in this image, so no end-to-end proxy-auth scenario was executed locally.
This branch has been through four independent passes: the original fix, a first adversarial
verification (which added T5–T9), a second, fresh adversarial verification at
46789dc9whichrebuilt the boundary ledger from the diff rather than from this document and added T10–T14, and a
fourth pass that re-ran the whole A/B at this head — base build (
git checkout 079789a903 -- packages/puppeteer-core/src/bidi/HTTPRequest.ts, fulltsc -b), the 14-test run, then the fixrestored, rebuilt, and the 14-test run again — purely to re-derive the transcripts in this document
instead of inheriting them. Result at
f3280903: base 4 pass / 10 fail, fixed 14 / 14.Every test count and every console block in Summary, Repro and Test evidence is from
that head. Across all four passes the production source has not moved:
git diff 46789dc9 f3280903 -- packages/puppeteer-core/src/bidi/HTTPRequest.tsis empty, so thecode both reviewers approved is byte-for-byte the code below this sentence; the fourth pass changed
no code at all.
Fork CI did run at this head, and covers exactly the gap the container cannot — these are
upstream's own workflows executing on our branch. At head
f3280903,gh pr checks 1 --repo askalf/puppeteer, re-read at 2026-09-14T19:42Z, reports 30 pass /1 fail / 6 skipping — the test matrix is run 34875915557
(
CI, success) plus 34875915556(
Docs, success); the skipping entries are the DevTools-CI and docs-only lanes that do not apply tothis path, and the single failure is the PR-title check discussed below.
f3280903Puppeteer Unit tests,[Required] Puppeteer Unit testsERR_MODULE_NOT_FOUNDfiles that fail in-container pass here)chrome-bidi (1-2),chrome-bidi (2-2)[Required] Chrome tests,chrome-headless,chrome-headless-shell,chrome-pipe(all shards)[Required] Firefox tests,firefox-headless(1-4 … 4-4),firefox-headful(1-4 … 4-4)[Required] Inspect codeDocker image test,[Required] Installation tests,npm installation, Angular schematics, browsers package[Required] Validate PR title[Required] Validate PR title— will not fail upstream. The job runsamannn/action-semantic-pull-requeston the PR title only, and the error is literally:[oss-candidate]is a fork-side label prefix; it breaks the conventional-commit parse. The titlesuggested at the bottom of this sheet —
fix(webdriver): handle continueWithAuth rejections— has noprefix and satisfies the action. Nothing in the diff is implicated: all five commit subjects already
use the conventional form (
fix(webdriver):,test(webdriver):).firefox-headful (2-4), previously flaky, is green here. At heada7f8c548that one shardfailed with
Run failed: 2 unexpected result(s)., bothTIMEOUTexpectations in[requestinterception-experimental.test] … "should navigate to URL with hash and fire requests without hash"and[target.test] … "Target.asPage() should return the same instance". Neither testauthenticates; neither goes through
network.authRequiredorcontinueWithAuth. It has sincepassed at
46789dc9and again atf3280903— two consecutive green runs, the first of whichdiffered from the failing head by three comments only. A result that flips with no behavioural
change is a flake, not a signal.
Worth noting: these are our fork's runs. Upstream will re-run everything on submission.
What no run here covers, and a maintainer may want: a live proxy-auth session where
continueWithAuthactually loses its request (page closed mid-challenge), confirming the log lineappears instead of the process dying. Boundaries row 19 also records a property measured but
deliberately not changed: a user-supplied
Loggerthat throws re-raises an unhandled rejectionfrom inside the new
.catch. That is the shape of every#logger?.(DEBUG_PREFIXES.error)?.(error)site in the tree, including the CDP twin this fix is modelled on, so it is flagged for maintainers
rather than fixed here.
Prior art
All run 2026-09-14 against
puppeteer/puppeteer:No open PR fixes this. puppeteer#14167 (merged, Sep 2025) is the closest prior work — it made
interception work after
continueWithAuthsucceeds, and did not touch the rejection path.puppeteer#14152 is closed as
upstream(a Firefox-side hang). puppeteer#14499 is open with no linked PR and isexplicitly not claimed by this change (see Bug). puppeteer#14151 is a feature request for a cancel API,
adjacent but different.
Note for whoever re-runs these:
gh search prs --state allis rejected (onlyopen|closed) — omit--stateto search both.Policy
docs/contributing.mdis the only contributing doc (CONTRIBUTING.md,AGENTS.md,CLAUDE.md,CODE_OF_CONDUCT.md,.github/CONTRIBUTING*are all absent). Greps forAI|LLM|GPT|copilot| generated|agent|CLA|sign-off|DCOreturn only these:AI stance: silent. No line about AI, LLM, generated or agent contributions anywhere in the
repo's contributor docs. Silence is not a ban, but it is also not permission — the disclosure is the
operator's to write (see below).
Required tooling, quoted from
docs/contributing.mdand all run above:The new test is placed next to the class it tests (
src/bidi/HTTPRequest.test.tsbesidesrc/bidi/HTTPRequest.ts), exactly as that line requires.All five commits on the branch use the repo's own scope convention observed in
git log(
fix(webdriver): …, cf. merged4e4daec603 fix(webdriver): don't store request on BrowsingContext (#14966)):fix(webdriver): handle continueWithAuth rejections,test(webdriver): cover continueWithAuth error logging,test(webdriver): cover continueWithAuth edge cases,test(webdriver): label the continueWithAuth control casesandtest(webdriver): cover the cancel arm of continueWithAuth error handling. The fork PR titleadditionally carries our
[oss-candidate]prefix, which is why theValidate PR titlecheck failson the fork — see Verification method.
The
.github/PULL_REQUEST_TEMPLATE.mdquestions, answered for the operator:packages/puppeteer-core/src/bidi/HTTPRequest.test.ts, 14 cases at thishead, 10 of which fail on base; the other 4 are controls, marked as such in the file.
Disclosure facts for the operator
Plain facts about what the AI assistance did, for you to phrase in your own words:
Page.authenticate()→addIntercept([authRequired])→network.authRequired→BidiHTTPRequest.#handleAuthentication,and concluded [Bug]: page.authenticate does not work over the webdriver bidi protocol puppeteer/puppeteer#14499's root cause is browser-side (Chrome does not send
network.authRequiredforproxy challenges) and not fixable in this repo.
void this.#request.continueWithAuth(...)calls.packages/puppeteer-core/src/bidi/HTTPRequest.ts).packages/puppeteer-core/src/bidi/HTTPRequest.test.ts— 14 cases atthe current head: 10 that fail on base and pass with the fix, plus 4 controls that pass on both
arms and are marked as controls in comments. The first 4 came with the original fix; a second
adversarial pass added the cancel-arm logging, non-
Errorreason, absent-logger, re-entrancy andempty-credentials cases; a third, independent adversarial pass (which rebuilt the boundary ledger
from the diff rather than from this document) added the five cancel-arm and reverse-order cases
T10-T14.
Loggerthatthrows re-introduces an unhandled rejection from inside the new
.catch. It is reported inBoundaries row 19 as a pre-existing, codebase-wide shape (every
#logger?.(...)?.(...)sitebehaves this way, including the CDP twin), not as something this PR introduces.
document is a predicted result.
workflows. The AI read them job by job; it did not run them.
cdp/NetworkManager.ts:440andapi/HTTPRequest.ts:736-752was read from thecurrent source, not assumed.
Boundaries
Every predicate, guard and truthiness check the diff adds or changes, rebuilt from the diff at head
f3280903. Test ids are the T1–T14 of Test evidence. The diff adds two.catchcallbacksand two
?.logger chains; rows are per-arm where the two arms are separately reachable.credentials && !this.#authenticationHandled(316) — unchanged by the diff, but selects which branch the new code runs incredentials = null(never authenticated /authenticate(null))else→cancel, rejection caughtcredentialsset, first challenge (#authenticationHandled === false)provideCredentials, rejection caughtcredentialsset, second challenge (#authenticationHandled === true)else→cancel(wrong-password re-challenge is not retried)calls[1]iscancel)credentials = {username: '', password: ''}— an object, so truthy even with empty stringsprovideCredentialswith the empty strings forwarded as-is; unchanged from basecredentials = null(→cancel), credentials set afterwards, second challenge#authenticationHandledis set only on the credentials arm, so the cancel does not consume the one-shot latch and the second challenge still provides credentials!this.#frameearly return (312) — unchangedcontinueWithAuth; no call, no rejection.catch(error => …)on theprovideCredentialsarm (327)continueWithAuthresolvesProtocolError('No such request with the given id')DEBUG_PREFIXES.error; handler resolves; no unhandled rejectionErrorreason (bare string) — i.e. the catch has no type discriminationerror instanceof ErrorErrorsubclass —TargetCloseError(page closed mid-challenge), which is the real-world trigger named in Bugcdp/NetworkManager.ts:440, and unlikehandleErrorit never re-throws (see Fix → alternatives rejected).catch(error => …)on thecancelarm (338)continueWithAuthresolvesProtocolErrorErrorreasonprovideCredentialscall rejects and whose subsequentcancelalso rejectsthis.#logger?.(DEBUG_PREFIXES.error)?.(error)— first?., credentials arm (331)#loggerisundefined?., cancel arm (339)#loggerisundefined?.(both arms)undefinedfor theerrorprefix (debug channel disabled — the normal case)undefined)DEBUG_PREFIXES.error.catchcallback and surfaces as an unhandled rejection — the failure mode this PR removes, re-entered through the log call. Not introduced or widened here: this is the shape of all ~20#logger?.(DEBUG_PREFIXES.error)?.(error)call sites in the tree, including the CDP twin atcdp/NetworkManager.ts:440that this fix is modelled on, andLoggeris a user-supplied callback documented to log. Left as-is deliberately: guarding it here alone would diverge from every siblingrejections = 1 [TypeError: logger blew up]on this head. Flagged for maintainers as a pre-existing, codebase-wide property rather than a regression of this PRawaitreplacingvoid(318, 334)EventEmitter.emitis synchronous and discards listener return values (common/EventEmitter.ts:136-142), and the sole registration isHTTPRequest.ts:129emit; the handler's promise now always resolvessetImmediateturns and assert Node reported nounhandledRejectionawaitordering / re-entrancyauthenticateevents emitted before the first settles#authenticationHandledis set before the await (317), so the second event still takescancel— awaiting opens no re-entrancy window['provideCredentials', 'cancel']).catchrequires a promise — synchronous-throw checkcontinueWithAuththrowing synchronously would bypass.catchbidi/core/Request.ts:317continueWithAuthisasync(returns a rejected promise) and carries no@inertIfDisposeddecorator — that decorator (util/decorators.ts:68) returnsundefinedwhen disposed and is applied only todispose()at:336Rows 6, 19 and 22 are the only ones without a committed test, each for a stated reason: row 6 is a
guard the diff does not touch, row 19 is a pre-existing codebase-wide property that this diff neither
introduces nor widens (measured and reported rather than silently asserted), and row 22 is a
source-read about the callee's shape.
Suggested upstream PR title
fix(webdriver): handle continueWithAuth rejections