fix(browser-session): require aggregate-issued lifecycle request authority - #317
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (10)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
Changes라이프사이클 포트 권한
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant BoundBrowserSession
participant DisposableContextPort
Caller->>BoundBrowserSession: bind_lifecycle_port(port)
Caller->>BoundBrowserSession: create_disposable_context()
BoundBrowserSession->>DisposableContextPort: create_disposable_context(request)
Caller->>BoundBrowserSession: destroy_disposable_context(authority)
BoundBrowserSession->>DisposableContextPort: destroy_disposable_context(request)
Merge Risk: ⚪ Minimal · up to No actionable current-head defect remains; complete the normal required checks before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 63.64% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 66 functions across 7 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head security finding on 97e0a4d875166ad733e78c1d3f213454ee615f01: the new lifecycle request is non-caller-constructible, but its port ownership is still self-asserted through public DisposableContextPortId::new(u64) plus DisposableContextPort::port_id(). Two distinct adapter instances can both report port_id=101; after A is bound, Browser Session's equality check will accept B as the same port. More strongly, A can relay the borrowed aggregate-issued create/destroy request to B, and B can satisfy the same scalar equality and reach remote lifecycle I/O even though B was never the aggregate-approved adapter instance. The current hostile test only uses 101 vs 102, so it proves mismatch rejection but not non-forgeable adapter ownership.
Required RED before this prerequisite can be GREEN: bind adapter A with id 101, then use distinct adapter B also claiming id 101; B must be rejected before create/destroy I/O and must not be able to consume/replay A's request. Do not repair this by documenting port-id uniqueness or randomizing a public scalar. The binding itself needs a non-caller-constructible Browser Session-approved/linear port capability or equivalent identity that a second adapter cannot self-select or replay. Keep remote BiDi identifiers outside Browser Session domain truth.
Separately, current hosted CI 34520503299 is still RED at rustfmt before tests/Clippy/rustdoc, and coverage measurement also fails; fix that operational RED without weakening gates after the authority model is corrected.
|
Exact-head RED evidence for Coverage job
So a distinct adapter B that self-reports the already-bound scalar port id can both reach create I/O and reach destroy I/O. This falsifies the current claim that aggregate-issued request + Repository status on this exact head is independently RED at |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head security finding on 9caf9bbe4228c443b7d5a4279831765a6a38765a: the current port-binding preflight still cannot guarantee the stated “reject before lifecycle I/O” boundary because bind_lifecycle_port / require_bound_lifecycle_port call the public trait method DisposableContextPort::port_id(&self) before issuing or validating the aggregate request. Rust &self is not a purity guarantee: an implementation can use interior mutability, a socket/client handle, atomics, or delegated state and perform remote side effects from port_id(). The rustdoc requirement that port_id be side-effect-free is therefore a convention, not an enforced security invariant. The same problem remains even after replacing the scalar id with a harder-to-forge value if Browser Session must first invoke an arbitrary adapter callback to discover/validate that value.
Required RED: use a hostile port whose identity/preflight callback records a lifecycle side effect, then exercise (a) a first create attempt and (b) a mismatched destroy/create attempt that Browser Session is expected to reject. Both must show zero adapter/browser mutation before an aggregate-issued authority exists and is validated. Do not satisfy this by documenting port_id() purity or by testing only the reviewed WebDriver adapter implementation.
Causal direction: remove authority-sensitive identity discovery from an arbitrary preflight callback. Establish the lifecycle-port binding through a non-caller-constructible composition/binding capability (or a session-owned/linear port wrapper) before lifecycle mutation, then invoke the adapter only under the already validated create/destroy request. If DisposableContextPort remains externally implementable, the contract must not rely on &self implying side-effect freedom. Preserve the current same-id/different-adapter hostile RED and the requirement that requests cannot be replayed into a different bound adapter. Keep remote BiDi identifiers outside Browser Session domain truth.
|
@coderabbitai review Please review exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Exact revision and PR diff summary =='
git rev-parse HEAD
git status --short
git diff --stat 6d87dff5dc572fbd74d06309d574a998f23cf02f..HEAD
git diff --name-only 6d87dff5dc572fbd74d06309d574a998f23cf02f..HEAD
printf '%s\n' '== Candidate lifecycle files =='
fd -t f -e rs . | while IFS= read -r file; do
if rg -q -i 'BrowserSession|BoundBrowserSession|DisposableContextPort|bind_lifecycle_port|DisposableContext(Create|Destroy)Request' "$file"; then
printf '%s\n' "$file"
fi
done
printf '%s\n' '== Rust API outline for candidate files =='
fd -t f -e rs . | while IFS= read -r file; do
if rg -q -i 'BrowserSession|BoundBrowserSession|DisposableContextPort|bind_lifecycle_port|DisposableContext(Create|Destroy)Request' "$file"; then
ast-grep outline "$file" --items all --type struct,trait,impl,function,method --match 'BrowserSession|BoundBrowserSession|DisposableContextPort|bind_lifecycle_port|DisposableContextCreateRequest|DisposableContextDestroyRequest' || true
fi
done
printf '%s\n' '== Lifecycle declarations, calls, and port-ID remnants =='
rg -n -C 4 --type rust \
'(\bbind_lifecycle_port\s*\(|\b(create|destroy)_(disposable_)?context\s*\(|\bDisposableContextPort\b|\bBoundBrowserSession\b|\bDisposableContextPortId\b|\bport_id\s*\()' \
.
printf '%s\n' '== Browser Session BiDi/protocol-specific references =='
rg -n -i -C 3 --type rust \
'(bidi|webdriver|pending|quarantin|remote.liveness|remote_liveness|accepted)' \
. \
-g '!**/test/**' -g '!**/tests/**' || true
printf '%s\n' '== Documentation and external fixtures relevant to binding =='
fd -t f . | while IFS= read -r file; do
if rg -q -i 'BoundBrowserSession|bind_lifecycle_port|DisposableContextPortId|distinct.adapter|hostile|linear.lifecycle|ADR.?.?0114' "$file"; then
printf '%s\n' "$file"
fi
doneLength of output: 147304 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Browser Session public types and lifecycle implementation =='
sed -n '1,720p' crates/originweave-browser-session/src/lib.rs
printf '%s\n' '== Focused external hostile tests =='
for file in \
crates/originweave-browser-session/tests/lifecycle_port_authority.rs \
crates/originweave-browser-session/tests/lifecycle_port_preflight_side_effect.rs \
crates/originweave-browser-session/tests/lifecycle_port_same_id_spoof.rs \
crates/originweave-browser-session/tests/sequential_incarnation_reuse.rs \
crates/originweave-browser-session/tests/destroy_failure_requires_recovery.rs \
tests/test_browser_session_lifecycle_contract.py
do
printf '\n== %s ==\n' "$file"
cat -n "$file"
done
printf '%s\n' '== Browser Session dependency boundary =='
browser_manifest="$(fd -t f '^Cargo\.toml$' crates/originweave-browser-session | head -n 1)"
printf '%s\n' "$browser_manifest"
cat -n "$browser_manifest"
printf '%s\n' '== Direct Browser Session crate imports =='
rg -n '^\s*use\s+originweave_[A-Za-z0-9_]+' crates/originweave-browser-session/src crates/originweave-browser-session/tests
printf '%s\n' '== Public mutable extraction and lifecycle helper reachability =='
rg -n -C 3 --type rust \
'(pub(\([^)]*\))?\s+(fn|struct|enum|trait)|fn\s+(create|destroy)_disposable_context_with_port|fn\s+(into_|as_mut|lifecycle_port_mut)|&mut\s+(self|P)|->\s*&mut)' \
crates/originweave-browser-session/src/lib.rs
printf '%s\n' '== Focused PR diff for implementation and contracts =='
git diff --find-renames --find-copies --unified=50 \
6d87dff5dc572fbd74d06309d574a998f23cf02f..HEAD -- \
crates/originweave-browser-session/src/lib.rs \
tests/test_browser_session_lifecycle_contract.py \
docs/adr/0114-browser-session-disposable-context-authority.md \
docs/traceability/browser-session-lifecycle-authority.md \
docs/uml/browser-session-lifecycle-authority.md
printf '%s\n' '== Changed documents =='
for file in \
docs/adr/0114-browser-session-disposable-context-authority.md \
docs/traceability/browser-session-lifecycle-authority.md \
docs/uml/browser-session-lifecycle-authority.md
do
printf '\n== %s ==\n' "$file"
cat -n "$file"
doneLength of output: 215228 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Repository-wide lifecycle API references =='
rg -n -C 3 --type rust \
'(\bcreate_disposable_context\s*\(|\bdestroy_disposable_context\s*\(|\bbind_lifecycle_port\s*\(|\blifecycle_port_mut\b|\binto_(inner|port|browser_session)\b|\bDisposableContextPortId\b|\bport_id\s*\()' \
.
printf '%s\n' '== Public methods on BrowserSession and BoundBrowserSession =='
rg -n -U --type rust \
'impl(?:<[^>]+>)?\s+(?:BrowserSession|BoundBrowserSession(?:<[^>]+>)?)\s*\{(?s:.*?)^\}' \
crates/originweave-browser-session/src/lib.rs || true
rg -n -C 2 --type rust \
'pub\s+(?:const\s+)?fn\s+(?:create_disposable_context|destroy_disposable_context|bind_lifecycle_port|lifecycle_port|browser_session|into_[A-Za-z_]+|[A-Za-z_]+_mut)\s*\(' \
crates/originweave-browser-session/src/lib.rs
printf '%s\n' '== Trait implementation locations =='
rg -n -C 2 --type rust 'impl\s+(?:[A-Za-z0-9_<>:]+\s+for\s+)?DisposableContextPort\b' .
printf '%s\n' '== Patch whitespace integrity =='
git diff --check 6d87dff5dc572fbd74d06309d574a998f23cf02f..HEAD
printf '%s\n' '== Exact-head PR checks and review state =='
gh pr view 317 --repo ContextualWisdomLab/OriginWeave \
--json number,headRefOid,isDraft,mergeStateStatus,reviewDecision,statusCheckRollup \
--jq '{number, headRefOid, isDraft, mergeStateStatus, reviewDecision, checks: [.statusCheckRollup[] | {name, status, conclusion, workflowName}]}'Length of output: 68284
I found no code-level issue in the requested security/API change.
The structural binding prevents ordinary callers from substituting or replaying a different adapter after binding. It cannot prevent a malicious trusted Exact-head verification is not complete. GitHub reports ✅ Action performedReview finished.
|
seonghobae
left a comment
There was a problem hiding this comment.
Merge-blocking integration finding on exact 9cde981899950b900698a17e7fa739af59f6bb4f: the linear BoundBrowserSession<P> closes caller-side port substitution, but the lifecycle contract still has no aggregate→adapter completion phase for creation, and DisposableContextCreateRequest is not unique per create attempt. Every create request in one Browser Session incarnation exposes only the same (BrowserSessionId, BrowserSessionIncarnation) pair, even though BrowserSession already reserves a distinct context epoch before remote I/O. After port.create_disposable_context(&request) returns, the aggregate may accept the handle or reject it as duplicate/ambiguous and enter RecoveryRequired, but DisposableContextPort receives no accept/reject completion carrying a per-attempt identity. A BiDi adapter implementing #316 therefore cannot deterministically move the exact remote tuple from pending→accepted or pending→quarantine without inventing adapter-local correlation/authority, keying only by the same session/incarnation, relying on call order, or prematurely authorizing before Browser Session validation.
Required RED before adoption: one bound port produces at least two creation candidates in the same Browser Session incarnation, retains them as separate pending protocol tuples, and Browser Session accepts one while rejecting the other (e.g. duplicate domain context/isolation). The adapter must promote only the accepted candidate and quarantine exactly the rejected candidate; neither candidate may collide/overwrite because their create requests are indistinguishable. No remote tuple may become authorizing before aggregate acceptance.
Causal repair should extend the Browser Session-owned transaction boundary, not move BiDi ids into this domain: mint a non-caller-constructible per-attempt lifecycle identity/capability (the already-reserved epoch is a natural candidate if its semantics fit), pass it in the create request, and provide an aggregate-issued accept/reject completion that the exact bound port consumes. The adapter keeps protocol tuples pending until that completion. Preserve structural port binding and the existing no-preflight/same-id hostile tests. #316 remains the owner of remote BiDi pending/accepted/quarantine data; #317 should only provide the domain transaction identity/completion contract it needs.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head recovery-correlation finding on 56a96ad8407b418d1775cfdf091519b57ad1e893: create-attempt identity is now correctly minted as attempt_epoch and adapters settle the exact pending tuple through DisposableContextCreateCompletion, but the recovery evidence produced when that settlement itself fails drops the transaction identity. UnsettledAdapterHandle(DisposableContextHandle) records only the remote handle, and DuplicateAdapterHandle(DisposableContextHandle) likewise does not identify the rejected create attempt. This is ambiguous in the exact case the per-attempt protocol was added to solve: a second create attempt can return a handle equal to an already accepted owned handle. The duplicate candidate and the pre-existing owned record then share the same isolation+browsing-context value, while the adapter's pending/quarantine ledger is keyed by the distinct attempt epoch. A separate recovery/reconciliation path cannot prove which protocol tuple failed to settle from the current public evidence alone, and enter_recovery_required() can also deduplicate the pre-existing sibling because DuplicateAdapterHandle(existing) == handle, collapsing two semantically distinct lifecycle facts into one value-level record.
Required hostile RED: in one Browser Session incarnation, accept attempt 1 for handle H; attempt 2 returns the same H; make the aggregate issue Rejected(attempt=2) and make the adapter fail that exact completion. Recovery evidence must retain enough non-authorizing transaction correlation to distinguish attempt 2's unsettled/rejected candidate from attempt 1's previously owned handle after the aggregate enters RecoveryRequired. A recovery owner must be able to address the exact pending/quarantined protocol transaction without reconstructing authority from raw ids or relying on call order.
Minimal causal direction: make create-related recovery evidence carry the aggregate-issued create-attempt identity (and, where needed, intended disposition) alongside the handle, e.g. a purpose-bounded recovery record rather than bare DisposableContextHandle. Preserve the existing handle evidence and do not expose a public constructor for attempt authority. RecoveryRequiredOwnedHandle should continue representing the previously accepted owned record independently; do not deduplicate it merely because a failed new candidate returned the same handle value. Keep BiDi tuple contents/persistence in #316; #317 only needs to preserve the domain transaction correlation required for deterministic recovery.
|
Exact-head CI Rust contracts Production coverage Nightly coverage compilation additionally warns that I returned #317 to Draft because the exact head is RED and review |
|
|
seonghobae
left a comment
There was a problem hiding this comment.
Fresh authoritative standards re-check finds a new documentation/provenance blocker on this exact head. The canonical W3C https://www.w3.org/TR/webdriver-bidi/ currently resolves to WebDriver BiDi Working Draft, 9 September 2026, dated version WD-webdriver-bidi-20260909, with 3 September 2026 as the previous version. ADR 0114, lifecycle traceability, product-gap baseline, and this PR body currently state that 24 August 2026 is the latest published Working Draft and that the 9 September snapshot could not be verified; that statement is now directly contradicted by the authoritative W3C TR.
Required repair: update the standards/APA trace to the 9 September 2026 published WD without changing the separately runtime-qualified protocol/browser compatibility pin. Treat this as standards provenance only: do not silently repin runtime behavior, weaken any Browser Session authority invariant, or mix it with #316 protocol tuple ownership. Repository contracts should assert the currently authoritative dated TR only if the project intentionally wants freshness to be a code gate; otherwise assert the semantic boundary and keep publication freshness in traceability/docs so a future W3C publication does not require production-code changes. Exact source blocker 5176486914 and CI RED remain independently unresolved.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head recovery-correlation gap remains broader than review 5176486914: CreateFailedUncertain also drops the aggregate-issued create-attempt identity. create_disposable_context_with_port reserves epoch before adapter I/O, but on DisposableContextCreateError::CreateFailedUncertain(isolation) it records only PartialCreationIsolation(isolation) when Some, and records no create-specific recovery evidence at all when None; it then enters RecoveryRequired. The adapter received the exact (session, incarnation, attempt_epoch) request and may still hold a pending/quarantined protocol transaction, but Browser Session's public recovery projection cannot identify which aggregate-issued attempt became uncertain without reconstructing it from next_epoch, call order, or adapter-local state.
This is especially important because ADR 0114 describes lifecycle failure evidence as lossless and #316 must later reconcile protocol pending/quarantined state without moving protocol tuple truth into Browser Session. An optional isolation id is not the transaction identity; None must not erase the fact of the unresolved attempt.
Please broaden the existing create-recovery repair rather than adding another unrelated mechanism. Hostile RED: accept attempt 1 normally; attempt 2 returns CreateFailedUncertain(None) (and separately Some(isolation)); after RecoveryRequired, non-authorizing recovery evidence must identify attempt 2 by its aggregate-issued attempt epoch while preserving any previously accepted owned handle as a separate recovery fact. Do not infer the attempt from next_epoch - 1, call order, or raw protocol ids. A purpose-bounded create-recovery record carrying attempt_epoch, failure/disposition kind, and optional candidate isolation/handle is sufficient; BiDi tuple contents and durable persistence remain #316/recovery-owner concerns.
Prerequisite repair for #312 and #314/#316, stacked on #229 exact
6d87dff5dc572fbd74d06309d574a998f23cf02f.Current exact head is
56a96ad8407b418d1775cfdf091519b57ad1e893. This PR is Draft during source repair and may be moved Ready only to obtain hosted exact-head verification; it is not merge-ready until repository contracts, canonical formatting, locked tests, strict Clippy, rustdoc/API docs, production function/line/region/branch coverage exactly 100%, and a fresh independent review accept this same exact head.Current repair delta:
5175575251:RecoveryRequirednow projects each indirectly invalidated Active sibling exactly once as non-authorizingRecoveryRequiredOwnedHandle, while preserving the triggering handle's cause-specific evidence without duplication. External hostile fixturerecovery_required_sibling_evidence.rsproves the two-context destruction-failure case.5175813759:BoundBrowserSession::finish(&mut self)validates completion without consuming the wrapper on rejection.ActiveContextRemainstherefore retains the exact bound adapter and ownership ledger so the caller can destroy/reconcile through the same owner and retry. External hostile fixturebound_session_abandonment.rsproves reject → same-owner cleanup → successful retry with no abandonment signal.5176486914remains a source blocker on this exact head: create-related recovery evidence (DuplicateAdapterHandle/UnsettledAdapterHandle) retains only the returned handle and loses the aggregate-issuedattempt_epoch. In the hostile case attempt 1 accepts handle H, attempt 2 returns the same H,Rejected(attempt=2)completion fails, and recovery must preserve attempt 2's transaction identity separately from attempt 1's previously owned H. Handle equality or call order must not collapse/reconstruct these two lifecycle facts; BiDi pending/quarantined tuple contents remain feat(bidi): bind current Browser Session authority to BiDi planning #316-owned.5176783252is a standards-provenance blocker: the canonical W3CTR/webdriver-bidicurrently identifies the 9 September 2026 Working Draft (WD-webdriver-bidi-20260909) as latest published, with 3 September as its previous version. ADR 0114 / traceability / product baseline currently say 24 August is latest and must be corrected. This changes standards traceability only; runtime-qualified protocol/browser revisions remain separately controlled and must not be silently repinned.docs/product-technical-gap-baseline.mdmust be synchronized with the recovery-correlation and standards-provenance repairs. The current baseline continuity note predates review5176486914and must not describe this PR as awaiting verification only.34578759212on this exact head is terminal RED. Repository contracts are 176/176 GREEN,cargo fmt --all --checkis GREEN, andcargo test --locked --workspace --all-targetsis GREEN. Strict Clippy fails atbind_lifecycle_portwithclippy::double_must_usebecause the function has a bare#[must_use]whileBoundBrowserSession<P>is already#[must_use]; the duplicate-handle fixture also has unnecessarymut. Remove the redundant annotation/dead mutability rather than allowing the lint. Rustdoc/API docs did not run.10190957720(sha256:a6757604038ab58d7d4d18a359f5e3d5d1c6495ddd40d37586298ccbe59eb155) reports branches 800/800, functions 699/699, lines 5920/5924, regions 7314/7318. Remaining uncoveredenter_recovery_required()branches intersect review5176486914; repair the recovery semantics rather than adding coverage-only branches or exclusions.34577862599on predecessorbd1bd857bba754ed06e3c326eeeba953f96faaa0passed Python repository contracts but failedcargo fmt --all --check. Its uploaded rustfmt artifact10190470645identified onlysrc/lib.rs,bound_session_abandonment.rs, andrecovery_required_sibling_evidence.rs; those exact formatter deltas were applied in this successor. Production coverage on that predecessor was cancelled when the PR returned to Draft for immediate repair, so no predecessor coverage result is promoted.d5046e76cb7555b448b728ea1bed9ba1ea8de8c3/ CI34573175780likewise passed Python repository contracts, then failed canonical formatting; its production coverage stopped during measurement because the two hostile lifecycle REDs above were still deliberately failing. Historical results do not transfer.Implemented lineage that remains valid:
BoundBrowserSession<P>consumes and privately retains the exact lifecycle adapter; there is no public raw port accessor, replacement-port lifecycle argument, self-asserted adapter identity, or generic raw adapter callback.BrowserContextEpochbefore remote create and carries it in opaqueDisposableContextCreateRequest; aggregate-issuedDisposableContextCreateCompletionsettles exact Accepted/Rejected candidates.AuthorizedContextOperationRequest<O>has no public constructor.AuthorizedContextOperationPortkeeps operation semantics adapter-owned while Browser Session validates currentPresentationMutationAuthoritybefore I/O and routes only to the same consumed adapter.BoundBrowserSession<P>manualDebugnever invokesP::fmtand redacts the adapter.TransportLossOwnedHandlerecovery evidence and is idempotent.Dropperforms no browser I/O and only increments a process-local abandonment signal. That signal is neither exact recovery evidence nor destruction proof.Current Browser Session merge blockers are source repair plus exact verification: repair review
5176486914with aggregate-issued create-attempt correlation while preserving previously accepted same-valued ownership as a distinct recovery fact; repair standards trace per5176783252; remove the exact Clippy/dead-mutability failures; synchronize ADR/traceability/product baseline; then obtain repository contracts → canonical formatting → locked tests → strict Clippy → rustdoc/API docs plus production function/line/region/branch exactly 100%, followed by a fresh independent review on the same head. Review5174248631is covered only if the current failed-finish retention evidence passes; no historical review is promoted across heads. Historical review5174624587remains retracted by5175126760.Buyer acceptance still open beyond this foundation: #316 WebDriver BiDi pending→accepted/quarantined integration and remote-liveness reconciliation, durable recovery persistence across crash/process restart, real browser-observed destruction/post-conditions on a current qualified Chromium lane, #299 3/3 replay, and protected-main release/SBOM/provenance/reproducibility/rollback.
No #229/main/#316 mutation, force/destructive restack, self-approval, bypass, workflow/ruleset/secret change, sandbox weakening, provider/model pin, coverage weakening, merge, tag, or release.