refactor(webkit): split WebKitClient internals while preserving facade (#706)#732
Conversation
Behavior-preserving extraction; same class names, constructor signatures, and instanceof semantics. No public API change. - New: src/webkit/errors.ts — ConnectionError, TimeoutError, ProtocolError, EvaluationError - client.ts now imports from ./errors and re-exports for backward compat - index.ts exports errors directly from ./errors (same external surface) - New: tests/unit/webkit-errors.test.ts — asserts name/code/message/instanceof + re-export identity Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the WebKit client by extracting several error classes into a new dedicated file, src/webkit/errors.ts, and adding unit tests to verify their behavior and re-exports. The reviewer suggested removing meta-comments from the new file that describe the refactoring process to keep the source code clean and focused on the implementation.
| // ========== Error Classes ========== | ||
| // Behavior-preserving extraction from client.ts. | ||
| // Same class names, constructor signatures, and instanceof semantics. |
There was a problem hiding this comment.
These comments appear to be notes about the refactoring process (e.g., "Behavior-preserving extraction") rather than documentation for the code itself. Since this information is already captured in the pull request description, it is better to remove these meta-comments from the source code to keep it clean and focused on the implementation.
| // ========== Error Classes ========== | |
| // Behavior-preserving extraction from client.ts. | |
| // Same class names, constructor signatures, and instanceof semantics. | |
| // ========== Error Classes ========== |
Behavior-preserving; same wire protocol; same error semantics. Extracts WebSocket / RDP message-passing layer from client.ts into src/webkit/protocol-transport.ts: - ProtocolTransport interface (adapter) used by WebKitClient to avoid circular dependencies and enable test doubles - WebSocketProtocolTransport concrete class owns the WebSocket, outer/inner pending-request maps, message-id generation, and Target.dispatchMessageFromTarget response routing - WebKitClient delegates send/connect/disconnect/isConnected to the transport; all public method signatures unchanged (facade preserved) - New tests/unit/protocol-transport.test.ts: happy path, out-of-order routing, TimeoutError, ConnectionError, ProtocolError propagation (outer + inner), domain event emission, disconnect rejection Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Behavior-preserving; same target/active-target semantics; per-target enabled-domain dedup preserved. TargetSessionManager owns knownTargets, activeTargetId, enabledDomainsPerTarget and Target.* event subscriptions. WebKitClient delegates all target state operations to the manager via the TargetCommandSender adapter interface (no circular deps). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Behavior-preserving extraction of high-level browser commands from client.ts into three new modules: - src/webkit/dom-input-scripts.ts: DOM script builders (tap, longPress, swipe, setValue, typeChar, focus, selectOption). Uses document.createTouch() / document.createTouchList() — never new Touch(). Uses prototype-walk value setter to avoid cross-realm TypeError. - src/webkit/evaluate.ts: evaluateValue<T> helper implementing the three-step WebKit eval pattern (Runtime.evaluate → Runtime.awaitPromise as separate command → Runtime.callFunctionOn for object serialization). - src/webkit/browser-commands.ts: BrowserCommands class taking a BrowserCommandSender adapter. Implements navigate, screenshot (Page.snapshotRect only — never captureScreenshot), click, longPress, swipe, type, scroll, press, dismissKeyboard, selectOption, readPage, querySelector/All, inspect, waitFor, getCookies/setCookies/clearCookies. WebKitClient becomes a thin facade: same public API, delegates every browser command to BrowserCommands. Same protocol calls, screenshot path, touch APIs, and per-target enabled-domain dedup preserved. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- screenshot: skip viewport eval roundtrip when caller provides clip - screenshot: surface underlying error in fallback message for diagnostics - press: use DigitN/KeyN code prefix per UI Events spec, omit for symbols - getCookies: exact domain match (with optional leading-dot tolerance) instead of permissive substring includes() - clearCookies: strip leading dot from cookie domain before building URL for Page.deleteCookie to avoid invalid `https://.example.com/` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex review noted that strict-equality filtering dropped subdomain cookies:
getCookies("example.com") would no longer return cookies scoped to
www.example.com. The previous includes() check was too permissive
(it matched another-example.com), and exact equality was too strict.
Match per RFC 6265 cookie scoping: a filter of `example.com` matches
`example.com`, `.example.com`, any subdomain (`www.example.com`), and any
parent domain whose scope still applies — but not `another-example.com`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the meta-comments at the top of src/webkit/errors.ts that described the refactoring process. The PR description already captures the behavior-preserving extraction context; the source file should focus on the implementation.
- Replace the brittle transport.emit monkey-patch with a formal
onProtocolEvent(handler) subscription on the ProtocolTransport
interface. The transport now relays raw RDP events to subscribers via
a dedicated channel, so transport-lifecycle events ('transport:close',
'transport:error') and EventEmitter housekeeping events
('newListener', 'removeListener') stay scoped to the transport.
- Auto-clear pending requests inside the WebSocket close handler so any
awaiting promises reject immediately with ConnectionError instead of
hanging until their per-request timers expire.
- Drop the (transport as WebSocketProtocolTransport) cast in
handleDisconnect. WebKitClient now reuses the public
transport.disconnect() path, which already clears pending state, so
the client no longer needs to know the concrete transport class.
- Cover the new behaviors with two unit tests (ws close auto-clears
pending; onProtocolEvent relays raw RDP events without leaking
lifecycle events).
- press: alphabetic-key fallback now sets keyCode to the uppercase ASCII value (65-90) per KeyboardEvent semantics on keydown/keyup, instead of emitting lowercase ASCII (97-122) for lowercase input - getCookies: drop subdomain match — cookies scoped to a subdomain of the filter host are not sent to that host per RFC 6265, so they should not be returned by a parent-host filter; also guard against empty cookie domains in the fallback path - clearCookies: skip cookies whose domain is empty (document.cookie fallback path) so we never construct `https:///` URLs for the Page.deleteCookie protocol command Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…error Codex flagged that emitting Node's standard 'error' event from the onProtocolEvent guard could terminate the process if no 'error' listener is registered. Switch to the existing transport:error lifecycle channel that WebKitClient already handles, and add a regression test that confirms a throwing subscriber surfaces on transport:error without ever hitting the default error event.
Codex flagged a P1 regression: when Page.getCookies is unsupported, getCookies() returns cookies with domain=''. The previous fix skipped those entries to avoid invalid `https:///` URLs but then silently returned, making clearCookies a no-op on proxies that lack the protocol command. Track whether any cookie was actually processed via the protocol path. If we collected cookies but none had a usable domain, fall through to the JS-based document.cookie clearing block instead of returning early. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5/5) Extract EventBridge class and typed payload interfaces (ConsoleMessage, RequestInfo, ResponseInfo, ErrorInfo, TargetCreatedPayload, TargetDestroyedPayload) from client.ts into src/webkit/events.ts. WebKitClient is now a pure facade: constructor wiring only, all branches delegation-only, zero business logic remaining. Behavior-preserving: same event names, same payload shapes, same emission timing. Typed interfaces exported from events.ts and re-exported via index.ts. New tests/unit/events.test.ts: 14 assertions covering transport forwarding, target lifecycle re-emission, and all five typed convenience listeners. client.ts: 550 → 496 lines (this PR); 1316 → 496 across the 5-PR series. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- attach(): make idempotent so repeat calls do not double-wrap transport.emit or stack duplicate target lifecycle listeners - targetSessionEmitter param: use the imported EventEmitter type for consistency with the rest of the file (drops the global NodeJS namespace) - on*() convenience listeners: attach the listener BEFORE awaiting enableDomain via a shared wireDomainListener() helper. Closes the race window where an event fires between the resolver send and the promise microtask. On enableDomain failure, detach the listener and surface the error so callers are not left silently subscribed to a domain that never woke up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The host parameter is typed `EventBridgeHost & EventEmitter` and EventBridgeHost.emit is part of the interface, so the EventEmitter prototype call is unnecessary indirection. Use the typed method directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/gemini review |
|
@codex review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…' into hermes/update-732
Resolve conflicts between the #706 webkit refactor series (1/5–5/5) and develop's ~57-commit advance. Key resolutions: - src/webkit/errors.ts: combine both versions (all 4 error classes) - src/webkit/evaluate.ts: use develop's fast-path (returnByValue:true, contextId option) as the canonical evaluateValue helper - src/webkit/dom-input-scripts.ts: use develop's options-object API (TapScriptOptions, LongPressScriptOptions, SwipeScriptOptions, SetValueScriptOptions, AppendCharScriptOptions) - src/webkit/client.ts: keep HEAD's thin facade; add dedup + cleanup logic to enableDomainForTarget and an enabledDomainsPerTarget accessor - src/webkit/browser-commands.ts: inline the comprehensive 3-step evaluate() (handles Promises via awaitPromise), use evaluateValue fast-path for screenshot viewport and navigation final-state batch read, update dom-input-script call sites to options-object API - src/webkit/index.ts: export both sets of public symbols - src/webkit/target-session.ts: add hasEnabledDomainForTarget, hasEnabledDomainsEntryForTarget, deleteEnabledDomainsForTarget helpers - src/mcp-server.ts: fix TS2554 — drop 5th arg from logAuditEntry calls (develop changed signature to 4 params) - src/security/audit-logger.ts: remove stale `status` reference - tests: update FakeTransport stubs for onProtocolEvent, update navigate and screenshot test mocks for new evaluateValue fast-path behavior Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50ae2b8078
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await this.evaluate( | ||
| buildSetValueScript({ selector, value, dispatchEvents: 'input-change' }), | ||
| ); |
There was a problem hiding this comment.
<select> elements
The new selectOption implementation now delegates to buildSetValueScript, which sets .value on any matched element and dispatches input/change events. Previously this path explicitly returned unless el.tagName === 'SELECT', so mis-targeted selectors were harmless. With this change, a selector that accidentally matches an <input>/<textarea> will overwrite user data and can trigger unintended form logic, which is a behavior regression for callers expecting select-only semantics.
Useful? React with 👍 / 👎.
…n suite Auth tools are tier-3 / high-risk and the test starts the MCP server over the HTTP transport. After the develop merge, the HTTP transport requires either an authToken or explicit insecure mode, and high-risk tools require explicit opt-in. Without those flags every JSON-RPC call returned 401/403 with the JSON-RPC error envelope, leaving res.result undefined and producing TypeError at the isError() helper. This is the same fix sibling PR #731 applied to other test files; mirror it here so the suite runs end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes the remaining test failures introduced by the develop merge: - auth/issue-167/mcp-server tests: pass httpHighRiskTools / httpInsecure to MCPServer.start() so tier-3 tools register and the HTTP transport doesn't 401 during the test. - http-high-risk-tools: spy fs.appendFileSync (the audit logger's current write path) instead of the deprecated fs.appendFile signature. - audit-logger: drop 'text' and 'value' from SENSITIVE_KEYS so legitimate type.text and select_option.value arguments are no longer over-redacted. - audit-logger: add optional 5th `status` parameter (matches the AuditEntry contract) and propagate from mcp-server's high-risk gate as 'denied' / 'allowed' / 'error'. - flutter-evaluate: assert against the new 'len=N' redacted format. - webkit-evaluate: drive the targetDestroyed cache-clear test through the protocol-transport event (handleMessage moved off the client during the #706 refactor; emit the parsed event directly instead). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a831782e70
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const result: MCPResult = await handler(sessionId, args); | ||
| if (this.auditLogEnabled || isHighRiskHttp) { | ||
| logAuditEntry(name, sessionId, args, undefined, result.isError ? 'error' : 'allowed'); | ||
| logAuditEntry(name, sessionId, args, undefined, 'allowed'); |
There was a problem hiding this comment.
Audit entries are now always written with status "allowed" after handler execution, even when the tool returns isError: true. In this codebase many tool handlers report operational failures via isError instead of throwing, so this change silently misclassifies failed executions as allowed and breaks error-rate/incident monitoring from audit logs. The status should continue to reflect result.isError in this non-throw path.
Useful? React with 👍 / 👎.
Keep the WebKit refactor behavior-preserving by restoring select-only semantics for selectOption, and keep high-risk MCP audit telemetry accurate when handlers return isError results without throwing.\n\nConstraint: PR #732 is a refactor against issue #706 and should not introduce behavior changes beyond review fixes.\nRejected: Inlining a separate selectOption script | would duplicate the shared prototype-walk value setter instead of extending the existing builder narrowly.\nConfidence: high\nScope-risk: narrow\nDirective: Keep selectOption restricted to SELECT elements unless a separate behavior-change issue explicitly widens it.\nTested: npm test -- --runInBand tests/unit/browser-commands.test.ts tests/unit/http-high-risk-tools.test.ts; npm run lint -- --quiet; npm test -- --runInBand selected WebKit/MCP suites; npm run build; npm test -- --runInBand; git diff --check\nNot-tested: live Safari simulator/manual smoke
Keep the WebKit refactor behavior-preserving by restoring select-only semantics for selectOption, and keep high-risk MCP audit telemetry accurate when handlers return isError results without throwing. Constraint: PR #732 is a refactor against issue #706 and should not introduce behavior changes beyond review fixes. Rejected: Inlining a separate selectOption script | would duplicate the shared prototype-walk value setter instead of extending the existing builder narrowly. Confidence: high Scope-risk: narrow Directive: Keep selectOption restricted to SELECT elements unless a separate behavior-change issue explicitly widens it. Tested: npm test -- --runInBand tests/unit/browser-commands.test.ts tests/unit/http-high-risk-tools.test.ts; npm run lint -- --quiet; npm test -- --runInBand selected WebKit/MCP suites; npm run build; npm test -- --runInBand; git diff --check Not-tested: live Safari simulator/manual smoke
shaun0927
force-pushed
the
refactor/706a-errors
branch
from
1991856 to
bdcda4e
Compare
shaun0927
changed the title
There was a problem hiding this comment.
💡 Codex Review
opensafari/src/security/audit-logger.ts
Lines 36 to 40 in bdcda4e
Restore redaction for generic text/value audit fields
Removing text and value from SENSITIVE_KEYS causes audit logs to persist raw free-form input from tool calls (for example type.text when entering passwords/OTPs or select_option.value carrying tokens), which is a regression in credential hygiene and can leak secrets to disk in args_summary. Because these fields are commonly used for user-entered data, they should remain redacted the same way other credential-like keys are.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Merge the latest develop changes so the WebKit refactor remains reviewable against the current base while preserving the new audit redaction posture and PR-specific audit-status/selectOption fixes. Constraint: develop advanced after the review fix and GitHub marked PR #732 conflicting. Rejected: Rebasing the full historical refactor stack | would rewrite many author commits and make the already-reviewed PR harder to audit. Confidence: high Scope-risk: moderate Directive: Preserve develop's free-form text/value audit redaction policy during future conflict resolutions. Tested: npm run lint -- --quiet; npm test -- --runInBand tests/unit/http-high-risk-tools.test.ts tests/unit/audit-logger.test.ts tests/unit/browser-commands.test.ts tests/unit/simulator-app-manager.test.ts tests/unit/simulator-lifecycle.test.ts tests/unit/simulator-ui-controller.test.ts tests/unit/device-catalog.test.ts tests/unit/simulator-errors.test.ts tests/unit/rotation-fallback.test.ts Not-tested: full suite/build after merge conflict resolution yet
|
Merge-readiness review completed for PR #732. This PR is valid for #706 because it keeps I also reviewed the automated feedback. The earlier source-comment cleanup and transport/event/cookie follow-ups were already addressed. Two latest Codex findings were still valid and are now fixed:
I also refreshed the PR title/body so they match the actual current scope, then merged the latest Validation evidence on the final head (
Architectural review status: CLEAR. The remaining risk is limited to live Safari/simulator smoke behavior, but the PR is a behavior-preserving refactor with focused unit coverage around the extracted boundaries plus green CI/build. I consider this merge-ready. |
2 participants
Implements #706: behavior-preserving WebKitClient internal extraction with the public BrowserBackend-facing facade kept compatible.
Intent
src/webkit/client.tshad accumulated protocol transport, target/session state, browser command implementations, DOM input script generation, event adapters, and error classes in one large module. This PR splits those responsibilities into focused modules while preserving the existing external API surface.Changes
src/webkit/errors.tsand keeps client/index re-exports compatible.src/webkit/protocol-transport.ts.src/webkit/target-session.ts.src/webkit/browser-commands.tsandsrc/webkit/dom-input-scripts.ts.src/webkit/events.ts.WebKitClientas the compatibility facade.develophardening changes.Review feedback addressed
onProtocolEventsubscription.selectOptionselect-only semantics so mis-targeted selectors do not mutate inputs/textareas.status: "error"when a handler returnsisError: truewithout throwing.Validation
Local validation after the latest fixes:
npm test -- --runInBand tests/unit/browser-commands.test.ts tests/unit/http-high-risk-tools.test.ts— 31 passednpm run lint -- --quiet— cleannpm test -- --runInBandselected WebKit/MCP suites — 196 passednpm run build— successnpm test -- --runInBand— 165 suites / 2502 tests passedgit diff --check— cleanCI is expected to be the final merge gate for platform smoke jobs.