feat(network): bind typed transport evidence to BiDi teardown - #255
feat(network): bind typed transport evidence to BiDi teardown#255seonghobae wants to merge 127 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Test-first peer-role checkpoint published at exact RFC 6455 §7.4.1 explicitly defines 1010 for the client and says the server does not use it; 1011 remains the defined server unexpected-condition code. IANA (registry last updated 2026-06-10) likewise lists 1010 Mandatory Ext. and 1011 Internal Error. Hosted CI Writer lease |
|
PR-state writer lease ACTIVE only for stale checkpoint repair on current exact |
|
PR-state-only lease |
|
Writer lease ACTIVE — exact 10d5e1f. I am first reproducing the published real-server Close 1010 test locally. If and only if it fails for the documented role-validation cause, scope is the minimal shared server-role status guard plus focused regression, full coverage, docs/CHANGELOG/AGENTS and visual verification. Preserve #252 parent, all masking/deadline/generation/evidence invariants and Draft state. No force push, workflow/ruleset/secret change, self-approval, merge or release. |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head review of the test-first server-1010 checkpoint: the realistic RED shape is appropriate, but the later production repair must preserve the distinction between wire-code validity and peer-role validity. RFC 6455 §7.4.1 defines 1010 as a legitimate client Close code and explicitly says the server does not use it; 1011 is the defined server unexpected-condition code. Therefore do not repair this by simply adding 1010 to the generic validate_close_frame invalid-code range while retaining the current "Close frame status code is not valid on the wire" diagnosis. That would encode a false global invariant and could later reject a valid client-originated 1010 path for the wrong reason.
After this exact realistic test actually executes RED, keep generic framing/registry validation separate and add the narrow server-to-client role check at the trusted client receive boundary (or make the existing helper explicitly peer-role aware). Reject server 1010 before any mirrored Close or final closure evidence, with a role-accurate bounded diagnostic; preserve server 1011, the existing 1016..2999 unassigned rejection, caller-owned masking entropy, repeated-control budget, shared operation deadline, exact connection generation, and clean-EOF requirement. No production mutation should precede the executed RED.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head hosted gate has now executed and confirms the intended RED on 10d5e1ff78d46bbad004d4e0971d0ffceac757fa; the earlier queued state is stale. Run 34185660439 Production coverage job 101933375768 reached the real loopback role test: server_close_1011_remains_valid_and_is_mirrored passed, while server_close_1010_is_rejected_before_reply_or_closure_evidence failed at the explicit no-reply assertion because OriginWeave mirrored server Close(1010). This is the documented peer-role defect, not a coverage-threshold failure; measurement stops on test exit 101 before diagnostics/enforcement. The causal production repair is therefore authorized under the active writer lease, but keep wire-code validity separate from server-role validity as in review 5137320615: 1010 remains a legitimate client-originated code, while a server-sent 1010 must fail at the client receive/peer-role boundary before reply or closure evidence. Preserve server 1011 and all existing unassigned-code, repeated-control, masking, deadline, generation, and EOF invariants.
The same exact run also exposes an independent test-source formatting gate: Rust contracts 101933375996 passes all 142 repository contracts, then cargo fmt --all --check fails only on the multiline formatting of the new assert!(reply.is_none(), ...) in webdriver_bidi_transport_close_role_validation.rs; workspace tests/Clippy/rustdoc are therefore not exact-head evidence yet. Canonical rustfmt artifact 10040955946 was emitted. Apply that mechanical formatting together with the minimal role-aware production fix, rerun the focused role fixture first, then full Rust contracts and exact production coverage. Do not transfer predecessor/local GREEN as acceptance.
seonghobae
left a comment
There was a problem hiding this comment.
One correction is needed in the test-first contract itself before the production repair is finalized. Exact webdriver_bidi_transport_close_role_validation.rs currently asserts that server 1010 returns WebDriverBiDiWebSocketTransportClosureError::Frame { source: WebDriverBiDiWebSocketFrameError::MalformedFrame { .. } }. That over-constrains the RED to the same false global invariant called out in review 5137320615: a Close(1010) frame is syntactically/wire-valid RFC 6455 data, but 1010 is client-only and therefore invalid specifically for this server→OriginWeave-client role.
The executed hosted RED is still valid because the primary post-condition failed first: the client actually mirrored 1010. In the minimal repair, update this secondary assertion with the production boundary rather than forcing the role rule into validate_close_frame. The exact topology already gives the narrow location: in WebDriverBiDiWebSocketTransportClosureObservation::observe_pre_close, after the validated opcode-0x8 frame yields peer_close_status_code and before write_close_frame, reject Some(1010) with a dedicated transport-closure role error (for example a bounded PeerCloseStatusNotAllowed { status_code } / equivalent), no nested MalformedFrame source. Then keep write_close_frame generic so a legitimate client-originated 1010 remains representable. The real peer test should assert (1) no reply, (2) the role-specific typed error, and (3) 1011 still mirrors and requires clean EOF. This preserves the distinction between WebSocket framing validity and BiDi endpoint-role admission instead of encoding policy in the frame serializer.
Co-authored-by: OpenAI Codex <noreply@openai.com>
|
Writer lease RELEASED after publishing exact 6b6c90e. The real-server 1010 test reproduced RED before the minimal role guard; 1010 now fails before reply/evidence and the 1011 control remains valid. Exact-head local coverage is functions 1105/1105, lines 11574/11574, regions 14787/14787, branches 1218/1218. 142 repository contracts, strict all-target/all-feature Clippy, canonical formatting, warnings-denied rustdoc and doctests pass. Actual isolated Edge visual inspection passed; the owned browser process was terminated. Hosted run 34188785932 is queued, so keep Draft and do not treat this as hosted, protected-main or release acceptance. No source writer lease remains. |
|
PR-state writer lease ACTIVE on exact |
|
PR-state-only lease |
|
Source writer lease ACTIVE on exact |
|
Source writer lease |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head review of the role-error repair: the current source now preserves the distinction required by RFC 6455 §7.4.1. Server-sent 1010 is rejected in the WebDriver BiDi client transport-closure owner before write_close_frame, as typed PeerCloseStatusNotAllowed { status_code: 1010 }; it is no longer misclassified as wire MalformedFrame. Generic frame validation/serialization remains unchanged, so a legitimate client-originated 1010 is not globally forbidden. The real loopback regression also asserts no 1010 reply, exact typed error/diagnostic with no causal frame source, and retained 1011 masked Close + clean EOF behavior. Compare from test-first ce4ef7188... to this head touches only the closure owner and role-validation integration test.
This review is not GREEN or approval. The test-first run was cancelled while queued after the ordinary successor commits, and current CI 34194095253 is still queued with no job steps. Keep Draft and do not transfer predecessor 6b6c90ed... GREEN. If current Rust/coverage gates fail, repair the exact failure before any restack/acceptance; process exit and profile cleanup remain separate #28 evidence.
|
Writer lease ACTIVE — exact |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head gate repair finding. Predecessor CI 34194095253 proves the production role boundary itself behaves correctly under the real loopback 1010/1011 tests, but exact coverage artifact 10043530514 (sha256:753ad7128f44cada2d9b8aa30693b122368833f938b15975a315ba8a72c60168) is functions 1105/1105, lines 11579/11579, branches 1218/1218, regions 14796/14797. The sole aggregate missing region is the false/panic arm generated by the duplicate inline assert!(matches!(...PeerCloseStatusNotAllowed { status_code: 1010 })) at webdriver_bidi_websocket_transport_closure.rs:477, while the production guard at lines 154–159 is executed. Current head leaves that source blob unchanged (5efb37b4...) and fixes the separate strict-Clippy expect_err failure in the public integration regression. Next minimal repair should remove the duplicate inline assertion/test-only coverage artifact or express the public error contract without manufacturing a permanently unexecuted assertion panic region; do not add a deliberately failing test, weaken coverage, or alter the 1010 production guard. Keep the real loopback integration test as the behavioral authority and require same-head Clippy/rustdoc/exact coverage before clearance.
|
Writer lease RELEASED — bounded hosted-gate repair published as ordinary child |
|
Writer lease ACTIVE — exact |
|
Writer lease RELEASED — bounded hosted-coverage repair published as ordinary child |
|
Writer lease ACTIVE — exact |
|
Writer lease RELEASED — bounded exact-coverage repair published as ordinary child |
Problem and preserved lineage
Session-end acknowledgement, WebSocket Close exchange, TCP closure, browser-process exit and profile cleanup require separate evidence. This Draft retains exact base #252
363a78e36e7690e9ed5bf49829567e00e2ec5d59and the connection-generation, fresh masking, 64-control budget and one operation-wide deadline protections. Current head:ca490312647516d94a8edc5854be1d5f77eb5b6d.Server Close-role repair
Real loopback predecessor
10d5e1ff78d46bbad004d4e0971d0ffceac757faexecuted hosted RED: a WebDriver/WebSocket serverClose(1010)was mirrored, while valid serverClose(1011)passed. RFC 6455 §7.4.1 defines 1010 as a valid client Close status and explicitly says it is not used by a server; generic frame/wire validity therefore remains separate from the known peer-role boundary.Test-first
ce4ef7188a289bca836cb8f00cd76737b4eb4196required the real 1010/1011 loopback contract to returnPeerCloseStatusNotAllowed { status_code: 1010 }. Production repair30b45879e8f3cc3b24244a85524f36ee8898c784added that role-specific transport-closure error beforewrite_close_frame, preservesError::source() == None, and leaves the frame serializer able to represent a legitimate client-originated 1010. The public loopback contract proves no 1010 reply while 1011 is still mirrored and followed by clean EOF.Exact coverage RCA and repair
9e51537946f85874e340b484843a80c82ef42b3drepaired the strict-Clippy failure in the public integration fixture. Its CI34195499595is GREEN for repository contracts, rustfmt, full workspace tests, strict Clippy and warnings-denied rustdoc, while Production coverage was one aggregate region short inside the duplicate inline 1010 assertion.a4371639d4977d98862339656996489780f00daathen removed that inline fixture, but hosted CI34198089883falsified that repair. Rust contracts job101970263206was exact-head GREEN. Production coverage job101970263501measured successfully then failed exact enforcement; artifact10045289607(sha256:d18709e2ea30f1d003444ed9e58cf77b716a72d7a68ad102dea89d5b5e841dd6) reported functions1104/1104, lines11558/11562, regions14760/14764, branches1217/1218. The missing coordinates were the production server-1010 role guard in the crate unit-test instantiation. The separate public integration binary executed that path, but LLVM coverage kept the crate unit-test instantiation separately summarized.Current exact
ca490312647516d94a8edc5854be1d5f77eb5b6dchanges only test compilation wiring inoriginweave-network: the existing realistic publicwebdriver_bidi_transport_close_role_validation.rs1010/1011 contract is also compiled into the crate unit-test binary through a test-only self alias/path. This avoids a second synthetic fixture and preserves the public test as the single behavioral source for no 1010 reply, exact typed role error, Display/source semantics, and the valid 1011 mirrored-Close/clean-EOF control. Production WebSocket behavior is unchanged.Exact-head GREEN — 2026-09-08
CI
34202965629completed success on exactca490312647516d94a8edc5854be1d5f77eb5b6d. Rust contracts job101985783879passed Python repository contracts, canonical rustfmt, workspace check, full tests, strict Clippy and warnings-denied rustdoc. Production coverage job101985783660passed measurement, diagnostics upload and exact production coverage enforcement. The shared-fixture repair is therefore verified on the same head; predecessor evidence is no longer needed for acceptance of this slice.Both inline review threads currently shown by GitHub are resolved. Keep Draft because this is a stacked child of #252 and because browser-process termination, task-profile cleanup and the complete real Chromium action → observed post-condition → evidence → teardown composition remain separate #28 buyer gaps. The protected parent stack must adopt the valid delta in dependency order; exact-head GREEN is not permission to bypass that stack or claim a release.
No merge, tag, release, workflow/ruleset/secret change, self-approval, force push or destructive restack is claimed.