Repository navigation
fix(browser-pay): report why an intercept arm failed - #1262
Conversation
Callers only saw intercept_unavailable when a target could not be armed. The failed event now carries the CDP method, numeric code, timeout flag, target type, and origin. closeTarget still runs. Remote error text is not included. A nested setAutoAttach failure, and an attachToTarget failure on an iframe that would have been armed, now close that target too. Captcha, extension, and worker targets stay open. Co-authored-by: Rimmely Ewan <ratataque@users.noreply.github.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Structured CDP errors packages/browser-pay/src/cdp.ts, packages/browser-pay/src/cdp.test.ts, packages/browser-pay/src/interceptor.event-redaction.test.ts, packages/browser-pay/src/interceptor.protocol.test.ts |
CDP command failures and timeouts now use CdpCommandError. The error includes a valid method name and optional integer code, and omits remote error text. Tests check timeout details and error-property redaction. |
Browser-step failure logging packages/browser-pay/src/interceptor.ts, packages/browser-pay/src/logger.ts, packages/browser-pay/src/interceptor.protocol.test.ts, packages/browser-pay/README.md, .changeset/browser-pay-intercept-diagnostics.md |
The interceptor logs selected CDP step failures with structured fields and sanitized origins. Logger exceptions are suppressed. Tests cover these paths, and the README and changeset describe the diagnostics. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant Interceptor
participant CDPConnection
participant PayLogger
Interceptor->>CDPConnection: Send Fetch.enable
CDPConnection-->>Interceptor: Reject with CdpCommandError
Interceptor->>PayLogger: Log failure details
Interceptor->>CDPConnection: Send Target.closeTarget
Suggested reviewers: antoine-berger
Merge Risk: 
0fe20
-
The reported logging failure appears contained at the reviewed head, with no identified issue preventing merge after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly and concisely describes the main change: reporting why browser interception arming failed. Description check The description is complete and follows the repository template. It explains the problem, scope, related work, validation results, compatibility impact, security considerations, AI assistance, and che… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is complete and follows the repository template. It explains the problem, scope, related work, validation results, compatibility impact, security considerations, AI assistance, and checklist status.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 6 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the method name,
And keeps the remote text out of frame.
A warning carries codes that pass,
While logger faults leave flow intact.
Fetch failures close the target’s door,
Then quiet ears hop off once more.
Comment @coderabbitai help to get the list of available commands.
The failed event reports only the CDP methods armFetch can fail on, plus the integer code, timeout flag, target type, and origin. closeTarget is unchanged. Nested attach failures stay swallowed. Co-authored-by: Rimmely Ewan <ratataque@users.noreply.github.com>
Network.enable is swallowed, so the event only names Fetch.enable. Cover a short command timeout and a non-integer CDP code that omits code. Co-authored-by: Rimmely Ewan <ratataque@users.noreply.github.com>
CdpCommandError includes the local method. Nested auto-attach, resume, existing-frame attach, and close log through the existing logger without changing control flow. Co-authored-by: Rimmely Ewan <ratataque@users.noreply.github.com>
The catch only runs after Network.enable is swallowed, so the method is always Fetch.enable. Co-authored-by: Rimmely Ewan <ratataque@users.noreply.github.com>
A consumer logger must not reject arming or skip detach. The logged method is the literal the caller passed. Co-authored-by: Rimmely Ewan <ratataque@users.noreply.github.com>
|
/devin review |
|
Starting Devin Review. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Only a plain Domain.method is named. Any other caller-supplied method keeps the generic failure and timeout messages. Co-authored-by: Rimmely Ewan <ratataque@users.noreply.github.com>
intercept_unavailable is the same event as before. An optional logger records why Fetch.enable failed, alongside the other swallowed steps. Co-authored-by: Rimmely Ewan <ratataque@users.noreply.github.com>
|
/devin review |
|
Starting Devin Review. |
There was a problem hiding this comment.
🟢 Approval recommended
The focused diagnostics preserve existing failure behavior and redaction boundaries, with regression coverage and no unresolved findings.
No open issues 🎉
What changed in this PR
Adds optional, sanitized diagnostics to browser-pay so callers can identify failed browser steps without changing public events or fail-closed behavior.
Changes:
- Logs selected browser-step failures with method, timeout status, integer error code, and sanitized target context.
- Adds structured CDP errors that name valid methods while excluding remote error text.
- Expands regression tests and documents the diagnostics in a patch changeset.
| File | Description |
|---|---|
packages/browser-pay/src/logger.ts |
Allows boolean log fields for timeout status. |
packages/browser-pay/src/interceptor.ts |
Logs selected failures without changing interception control flow. |
packages/browser-pay/src/interceptor.protocol.test.ts |
Tests diagnostics, redaction, and throwing-logger behavior. |
packages/browser-pay/src/interceptor.event-redaction.test.ts |
Verifies safe error messages and unchanged event shapes. |
packages/browser-pay/src/cdp.ts |
Adds sanitized command errors with method and failure metadata. |
packages/browser-pay/src/cdp.test.ts |
Tests structured timeout errors. |
packages/browser-pay/README.md |
Documents optional logging and method-aware errors. |
.changeset/browser-pay-intercept-diagnostics.md |
Records a browser-pay patch release. |
Review effort: Balanced
|
interceptor.ts:461: async logger rejection escapes the guard and crashes Node; reproduced against head, while base exits normally. |
Co-authored-by: Rimmely Ewan <ratataque@users.noreply.github.com>
Primary change type
Problem and motivation
When arming a target fails, the caller only saw
failedwith reasonintercept_unavailableand the target closed. The public event stays that shape. An optional logger can now record which browser step failed, without the browser's error text.Summary and scope
InterceptorEventis unchanged.intercept_unavailableis still{ type: "failed", url, reason: "intercept_unavailable" }, and the target is still closed withTarget.closeTarget.loggerreceivesbrowser step failedfor aFetch.enablearm failure and for swallowed nestedTarget.setAutoAttach,Runtime.runIfWaitingForDebugger,Target.getTargets,Target.attachToTarget, andTarget.closeTarget. Fields are the local method, an integercodewhen the browser sent one,timedOut, andtargetType/ origin-onlyoriginwhen known. The default logger is silent. A throwing logger does not change control flow.CdpCommandErrornames a plainDomain.method(CDP command failed: Target.setAutoAttach). Any other method string keeps the generic message and leavesmethodundefined. Remote error text is not included.Network.enable,Target.setDiscoverTargets,Fetch.disable, and detach-time sends stay silent. No new events, states, or close paths.Related work
Related issue or discussion: N/A
Validation
Compatibility and release impact
CdpCommandErrormessages name a plain CDP method.PayLoggerfield values may be boolean (timedOut). Fail-closed close behaviour is unchanged.@sapiom/browser-paypatch.Security
will follow the
Security Policy for
private reporting.
Logs and errors do not include card data, request bodies, headers, a URL beyond origin, or the browser's error text. A method name is included only when it is a plain
Domain.method.Decision for Ewan
Log-only. The public event is not extended. Opt-in debug flag exposing raw CDP error text: no.
AI assistance
A Cursor cloud agent wrote the change. It was checked with the package typecheck, lint, and Jest suite.
Checklist
CONTRIBUTING.md, and this contribution follows the direct-PR or issue-first policy.any N/A checks above.
Summary by CodeRabbit