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
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
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
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
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
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
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.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
New current-head resource/performance finding: destroyed contexts remain permanently in BrowserSession.contexts, and every later create linearly scans all historical records for isolation aliasing before also checking the BTreeMap key. A long-lived governed session that repeatedly creates → proves destruction → creates again therefore grows hot ownership state without bound and makes creation cost O(total historical contexts), with cumulative O(n²) scan work. This is especially problematic for an enterprise browser runtime where disposable contexts are expected to be churned, and it conflicts with the runtime/resource-limit objective even though authority safety is otherwise monotonic.
The stale-authority invariant does not require retaining every destroyed record in the hot ownership map: PresentationMutationAuthority is bound to session incarnation + monotonically reserved context epoch + isolation + browsing context, so after proven destruction a later reused remote identity can still reject an older authority by epoch mismatch. If immutable audit/provenance retention is required, keep it out of the mutable hot ownership index or bound it explicitly; do not preserve unbounded tombstones in the command-authority structure.
Hostile RED: run a large sequential create → destroy-success cycle with unique handles under one session and prove (a) the hot ownership set stays bounded by unresolved/live ownership rather than total history, (b) an authority from an earlier destroyed cycle remains rejected before adapter I/O even if the adapter later reuses the same browsing-context/isolation values, and (c) create validation does not degrade linearly with the count of already-destroyed historical contexts. Minimal causal fix can remove or compact proven-destroyed records while relying on incarnation+epoch for stale-authority rejection, or split bounded active ownership from a separate immutable audit/provenance sink. Do not weaken duplicate detection for concurrently live/uncertain ownership.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
AuthorizedContextOperationRequest<O>가 current authority 검증 뒤 exact handle과 operation만 adapter에 전달하고 PresentationMutationAuthority::context_epoch를 버립니다. 따라서 같은 (session, incarnation, isolation, browsing_context)에서 epoch 1→2→3으로 authority가 회전해도 adapter/evidence 쪽에서는 각 실행이 어느 aggregate-issued epoch의 fresh validation을 통과했는지 구분할 수 없습니다. 즉 synchronous call 자체는 stale authority를 막더라도 #316이 요구하는 fresh-authority-at-I/O 및 durable provenance를 exact epoch에 결박할 수 없고, adapter-side queued/retry/evidence가 raw target tuple만으로 서로 합쳐질 위험이 있습니다.
Hostile RED를 추가해 주세요: context 생성 후 epoch를 2로 advance하고 authorized operation을 실행한 뒤, 다시 epoch 3으로 advance해 두 번째 operation을 실행합니다. Adapter fixture가 두 호출에서 각각 정확한 BrowserContextEpoch(2)와 (3)을 관찰·기록해야 하며 stale epoch 2 재사용은 adapter I/O 전에 0-call로 거절돼야 합니다. GREEN은 opaque AuthorizedContextOperationRequest에 Browser Session이 검증한 exact context epoch를 non-authorizing correlation field로 포함하고 accessor를 제공하는 정도면 충분합니다. 이 값은 adapter addressability/evidence correlation일 뿐 별도 mutation authority나 durable bearer token으로 사용하면 안 됩니다. BiDi-specific pending/accepted/quarantined persistence와 remote-liveness는 계속 #316 owner에 둡니다.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head destruction-provenance finding on 56a96ad8407b418d1775cfdf091519b57ad1e893, distinct from authorized-operation review 5178435963: Browser Session validates the full current PresentationMutationAuthority, including context_epoch, immediately before destruction, but DisposableContextDestroyRequest carries only (browser_session, incarnation, DisposableContextHandle). If destruction is unproven, BrowserSessionRecoveryEvidence::UnprovenDestruction likewise retains only the handle. The exact aggregate-issued authority epoch that authorized the destructive I/O is therefore discarded at both the adapter boundary and the recovery projection.
This does not currently let a stale authority through—the stale authority is rejected by context_for_authority_mut before I/O—but it breaks exact causal provenance and deterministic reconciliation for the destructive path. A reviewed BiDi adapter/recovery owner can prove which user-context handle was targeted, but not which Browser Session authority generation was freshly validated for the unproven browser.removeUserContext attempt. That becomes materially ambiguous after one or more advance_context_epoch transitions on the same live handle and conflicts with the stack's requirement that fresh authority be attributable at browser I/O rather than reconstructed from call order/private aggregate state.
Required hostile RED: create one context at epoch 1, advance it to epoch 2, then force destroy_disposable_context to return DestroyFailed. The adapter fixture must observe exact epoch 2 in the opaque destroy request, recovery evidence must preserve the same non-authorizing (handle, epoch 2) destruction-attempt correlation, and stale epoch 1 must still produce zero destroy I/O. Repeat with another context/epoch to prove evidence is not inferred from a global latest epoch or call order.
Minimal causal direction: carry the already-validated BrowserContextEpoch in DisposableContextDestroyRequest as non-authorizing correlation and preserve it with UnprovenDestruction (or a purpose-bounded destroy-recovery record). Do not turn the epoch into a serializable bearer capability, do not move BiDi tuple truth into Browser Session, and do not treat command ACK as destruction proof. The same bound adapter and existing fail-closed validation remain authoritative.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head recovery-handoff finding on 56a96ad8407b418d1775cfdf091519b57ad1e893: BoundBrowserSession<P> now structurally owns the only accepted lifecycle adapter, and its #[must_use] contract says unresolved ownership must be finished or handed to recovery. However there is still no public, non-authorizing handoff path that preserves that exact bound adapter together with the Browser Session recovery ledger once the aggregate enters RecoveryRequired or TransportLost.
This is not just an API-completeness concern. context_for_authority_mut() calls require_active(), so after a create/destroy uncertainty or transport loss, normal destroy/authorized-operation paths are intentionally fail-closed. browser_session() exposes only an immutable borrow, port is private, and dropping the wrapper merely increments the process-local abandonment counter before the concrete adapter value is dropped. A separately reviewed recovery owner therefore cannot take custody of the exact adapter plus BrowserSessionRecoveryEvidence; #316 cannot deterministically reconcile its pending/quarantined BiDi tuple through the same bound adapter without adding an ambient/second-adapter path, which this PR explicitly forbids.
Required hostile RED: create an accepted context, force destruction to DestroyFailed (and separately exercise transport loss), observe RecoveryRequired/TransportLost, then transfer the unresolved session into a recovery-only owner. Prove the recovery owner still holds the exact same adapter instance and exact recovery evidence, while ordinary presentation mutation/create paths remain unavailable. Dropping that recovery owner unresolved may count as abandonment; explicit handoff itself must not lose evidence or imply browser cleanup.
Minimal causal direction: add a purpose-bounded consuming transition such as into_recovery(self) -> BoundBrowserSessionRecovery<P> (name not prescribed) that retains the original wrapper/adapter and exposes only reviewed reconciliation capabilities. Do not expose P raw, do not reconstruct an adapter from ids, do not perform browser I/O in Drop, and do not let the handoff mint presentation authority. Protocol-specific BiDi recovery commands/persistence remain #316-owned; #317 only needs a lossless same-owner domain handoff boundary. Until such a path exists, the current statement that unresolved ownership can be “handed to recovery” is not implementable by consumers.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head compatibility/security finding on 56a96ad8407b418d1775cfdf091519b57ad1e893: DisposableIsolationId::parse rejects any browser-issued isolation identity whose Rust UTF-8 byte length exceeds 4096 (TooLong). The current Browser Session docs map this value one-to-one to WebDriver BiDi browser.UserContext, but the current W3C WebDriver BiDi Working Draft (9 September 2026) defines browser.UserContext as text and requires it to uniquely identify a user context; it does not define a 4096-byte protocol limit for this identifier. See https://www.w3.org/TR/2026/WD-webdriver-bidi-20260909/#type-browser-UserContext and browser.createUserContext.
This hard-coded domain limit can therefore make OriginWeave reject a protocol-valid browser-issued identity solely because it crossed an implementation-chosen threshold. That is especially problematic here because the value is remote lifecycle addressability needed for exact destroy/recovery; refusing to represent it can turn a successfully created remote user context into an ownership/recovery failure. The use of value.len() also makes the cutoff UTF-8-byte based rather than a specification-defined text constraint.
Required RED: a reviewed lifecycle adapter returns a unique, otherwise valid browser.UserContext whose encoded length is >4096 and a valid browsing context. Browser Session must not lose exact remote ownership merely because of an arbitrary local identifier-length constant. If resource-exhaustion protection is required, bound the authoritative protocol/frame/resource boundary with a cited standard/runtime limit or an explicit deployment policy and preserve lossless recovery evidence; do not silently redefine the semantics of browser.UserContext with an uncited constant.
Minimal causal repair: remove this arbitrary protocol-identity cap, or replace it with a documented/versioned bound backed by an authoritative WebDriver BiDi/Chromium/transport limit plus tests demonstrating that all accepted remote identities remain losslessly representable for create/destroy/recovery. Keep surrounding-whitespace/control-character handling only where it is actually justified by the remote identifier contract; do not normalize or truncate browser-issued identity. This is independent of the existing create-attempt/authority-epoch/recovery-handoff blockers and should not weaken any fail-closed authority rule.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head repository-contract blocker on eb2701bcd41aca1e442e8919e8dcef7bde805d47: ADR 0114 was correctly currentized to the W3C 9 September 2026 WebDriver BiDi WD, but tests/test_browser_session_lifecycle_contract.py::test_architecture_decision_and_traceability_are_explicit still requires the literal token WD-webdriver-bidi-20260824 to appear in ADR 0114. The current ADR no longer contains that token. Therefore the next non-skipped exact-head repository-contract run will fail before this branch can earn fresh GREEN, independently of the intentional user_context_identity_length.rs RED.
Repair the contract causally with the standards policy rather than reintroducing stale prose into ADR 0114. If publication freshness is intentionally a repository gate, assert WD-webdriver-bidi-20260909 and synchronize traceability/product baseline in the same source-writer change. If the repository test is meant to protect the semantic boundary rather than every W3C publication date, assert the authoritative-link/semantic contract and keep dated freshness in traceability so the next W3C editorial publication does not create an unrelated code-contract failure. In either case, do not weaken the full repository gate and do not count the current Draft-policy skipped CI as evidence.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head capability leak: BoundBrowserSession::browser_session() is documented as the read-only policy/ACL view, but it returns &BrowserSession, and BrowserSession::presentation_authority(&self, BrowsingContextId) is public. After one owned context exists, code that was given only that purportedly read-only aggregate reference can call bound.browser_session().presentation_authority(raw_context_id) and obtain a live PresentationMutationAuthority. That contradicts this slice's core invariant that raw browser identifiers do not become caller-mintable mutation authority and collapses the read-model/capability boundary without any adapter I/O.
Keep this blocking. Hostile acceptance: after normal create, expose only the API intended for read-only policy/ACL inspection and prove external code cannot obtain/clone a presentation capability from that view using only BrowsingContextId; the normal bound owner must still be able to issue current authority through its explicit capability surface. Minimum causal repair is to stop exporting the aggregate method through the read-only view (for example, make BrowserSession::presentation_authority non-public and/or return a dedicated non-authorizing BrowserSessionView from browser_session()). Do not replace this with a runtime check: the unwanted authority-minting path should be absent from the public type surface. Also keep recovery identities purpose-bounded rather than broadening the read-only projection while repairing it.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
5180191348의 browser.UserContext 호환성 결함은 4096-byte 상한만 제거해서는 닫히지 않습니다. current exact head의 DisposableIsolationId::parse는 여전히 (a) empty string, (b) leading/trailing whitespace, (c) Unicode control character를 각각 Empty/InvalidCharacter로 거절합니다. 그런데 현재 authoritative WebDriver BiDi WD(2026-09-09)는 browser.UserContext = text이고 user-context id를 “unique string set upon creation”으로만 정의합니다. 이 CDDL text에는 .size, .regexp, non-empty, trim, control-character 제한이 없습니다(RFC 8610 standard prelude에서 text = tstr). 따라서 이 lexical restrictions 역시 WebDriver BiDi domain truth가 아니라 OriginWeave가 임의로 추가한 조건입니다.
이 값은 remote lifecycle addressability이므로 adapter가 browser-issued id를 받은 뒤 표현 단계에서 거절하면 이미 생성됐을 수 있는 user context의 exact browser.removeUserContext/recovery 주소를 잃게 됩니다. 일반적인 ‘sanitization’도 허용하면 안 됩니다. 공백을 trim하거나 control을 drop/normalize하면 다른 remote identity로 바뀔 수 있습니다.
user_context_identity_length.rs 수리를 넓혀 hostile cases를 추가해 주세요: otherwise-valid remote result가 "", " context ", 그리고 JSON에서 escape되어 전달된 control-containing text(예: decoded "ctx\n")를 user-context id로 반환했을 때 DisposableIsolationId가 byte-for-byte/Unicode-scalar-for-scalar 동일한 string을 보존하고 destroy/recovery addressability까지 유지해야 합니다. 특정 Chromium/runtime qualification이 더 좁은 identifier grammar를 실제로 보장한다면 그 제약은 versioned adapter qualification/deployment policy로 증명하되, remote create 이후 exact id 보존을 막는 domain parser로 구현하지 마십시오. 이 finding은 authority/recovery blockers를 완화하지 않으며, browser.UserContext one-to-one mapping을 계속 주장하는 한 lexical normalization/rejection도 함께 제거하거나 authoritative 근거로 재정의해야 합니다.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Merge-blocking lifecycle-invalidation gap on exact f73cc5def267b99f43986cd3c504b86cb3d489d7: #312's Browser Session invariant says navigation/renderer replacement invalidates stale presentation authority unless the lifecycle explicitly re-establishes it, but the current foundation has only caller-invoked advance_context_epoch(BrowsingContextId). There is no protocol-agnostic Browser Session transition for an observed navigation/document/renderer replacement. Consequently an epoch-1 PresentationMutationAuthority can remain current across a real remote navigation if no caller happens to rotate the epoch, and execute_authorized_context_operation can then validate that stale pre-navigation authority and reach adapter I/O.
W3C WebDriver BiDi 9 September 2026 exposes browsingContext.navigationStarted as an observable navigation-start event. #316 already correctly states that local Rust lifetimes are not remote-liveness proof; the same principle applies to navigation freshness. Required hostile RED: create an owned context and retain authority epoch 1; record a browser-observed navigation for that exact owned context through a Browser Session-owned, protocol-agnostic invalidation transition; then prove the retained epoch-1 authority is rejected before adapter I/O. The event itself must not mint a replacement mutation capability. A fresh authority may be explicitly re-established only through the owning Browser Session policy after invalidation. Foreign/unknown context events must not invalidate another owner, and repeated delivery must not keep manufacturing epochs or evidence solely because an adapter retried an event.
Keep the mapping direction one-way: #316 may map the reviewed BiDi navigation event (and separately a qualified renderer/document-replacement signal) into the Browser Session invalidation operation, but WebDriver BiDi navigation ids/event payloads remain adapter addressability/evidence, not Browser Session authority. Do not solve this by asking callers to remember to call advance_context_epoch; freshness must be causally tied to the observed lifecycle transition.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Fresh standards verification found a provenance regression that is still present on this exact head. The W3C search result for the TR identifies the latest published WebDriver BiDi Working Draft as 24 August 2026 (WD-webdriver-bidi-20260824), while the current Editor's Draft is 9 September 2026 at https://w3c.github.io/webdriver-bidi/. The branch currently states that 9 September is the latest W3C-published Working Draft and tests/test_browser_session_lifecycle_contract.py now hard-requires WD-webdriver-bidi-20260909; that conflates Editor's Draft freshness with W3C TR publication status.
Please repair the traceability model rather than merely changing a date literal: keep separate fields/claims for (a) latest published W3C TR = WD-webdriver-bidi-20260824, and (b) current Editor's Draft observed 2026-09-09. If runtime qualification pins a different revision, keep that third axis separate too. Update ADR 0114, traceability/product baseline, repository contract, and PR body consistently. Do not mark the 9 September Editor's Draft as a published WD. The navigationStarted/UserContext semantics used by the active findings remain available in the current Editor's Draft, so this correction changes provenance status, not the underlying lifecycle finding.
1 participant
Prerequisite repair for #312 and #314/#316, stacked on #229 exact
6d87dff5dc572fbd74d06309d574a998f23cf02f.Current exact head is
f73cc5def267b99f43986cd3c504b86cb3d489d7. This PR remains Draft during source repair and 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 fresh independent review all accept the same exact head.Active Browser Session blockers:
5176486914: duplicate/unsettled create recovery loses aggregate-issuedattempt_epoch; accepted same-valued ownership and a later failed attempt must remain separate lifecycle facts.5177440777:CreateFailedUncertain(Some/None)must preserve the exact aggregate-issued create attempt identity even when no complete handle exists.5178435963:AuthorizedContextOperationRequestmust carry the already-validatedBrowserContextEpochas non-authorizing execution/provenance correlation; stale authority remains zero-I/O.5178943747: destroy request and unproven-destruction evidence must preserve the exact validatedBrowserContextEpoch; command ACK is not destruction proof.5179537115:RecoveryRequired/TransportLostneeds a purpose-bounded consuming handoff preserving the exact same adapter plus exact recovery evidence without restoring ordinary mutation authority. No rawP, second adapter, Drop I/O, or id reconstruction.5177951332: proven-destroyed history must not make command-authority hot state grow unbounded or create validation degrade with historical tombstones. Keep live/uncertain duplicate detection and stale-authority rejection; separate audit history from hot ownership state.5180191348: Browser Session mapsDisposableIsolationIdone-to-one to WebDriver BiDibrowser.UserContext, but production currently rejects UTF-8 byte length >4096 although the 9 September 2026 W3C WD definesbrowser.UserContextastextand no such protocol limit. Commit9818ada98cbdb9ed4054e96b7c7c4f3e52d51f3aadds intentional REDuser_context_identity_length.rs; otherwise-valid remote identity must remain losslessly representable for exact destroy/recovery. Resource bounds belong at a cited protocol/frame/runtime/deployment boundary, not an uncited domain constant.Standards/repository-contract repair completed on this lineage:
WD-webdriver-bidi-20260909, previous 3 September) without silently repinning the separately qualified runtime revision.9c17e7c8d6e491a2610e7cc5b48089a6980bfeberemoves the stale repository-contract requirement forWD-webdriver-bidi-20260824and gates the ADR onWD-webdriver-bidi-20260909instead.f73cc5def267b99f43986cd3c504b86cb3d489d7synchronizesdocs/traceability/browser-session-lifecycle-authority.md, records the arbitrary 4096-byte user-context ceiling as an open active-branch mismatch, and keeps standards freshness separate from runtime qualification.docs/product-technical-gap-baseline.mdstill contains the stale 24 August continuity note and must be currentized before documentation is code-current; do not treat the trace/ADR repair alone as documentation completion.Already repaired lineage remains valid:
RecoveryRequiredsibling exact-handle evidence (5175575251), non-consuming failedfinish(&mut self)same-owner retry (5175813759), structural single-adapter binding with no raw port accessor, aggregate-issued create attempt/completion boundary, redacted adapterDebug, idempotent transport-loss evidence, and non-I/O abandonment signaling.Hosted verification:
56a96ad8407b418d1775cfdf091519b57ad1e893/ run34578759212: repository contracts 176/176 GREEN, fmt GREEN, locked workspace tests GREEN, strict Clippy RED (double_must_useplus unnecessarymut), rustdoc not reached.10190957720(sha256:a6757604038ab58d7d4d18a359f5e3d5d1c6495ddd40d37586298ccbe59eb155) measured branches 800/800, functions 699/699, lines 5920/5924, regions 7314/7318. Repair semantics; no exclusions or coverage-only branches.9818ada...push CI34614061293andeb2701b...push CI34614356319were Draft-policy skipped, not GREEN evidence. No newer exact-head GREEN is claimed forf73cc5d....github.com; this is not a code result and is not promoted as verification evidence.Shortest causal path: create-attempt recovery correlation → operation/destroy epoch correlation → same-adapter recovery takeover → bounded hot ownership state → remove/replace arbitrary user-context cap with an authoritative boundary and turn
user_context_identity_length.rsGREEN → product-baseline sync → Clippy/dead-mutability repair → repository contracts → fmt → locked tests → strict Clippy → rustdoc/API docs → function/line/region/branch 100% → fresh independent review → ordinary #317 adoption → #316 non-force restack/remote-liveness reconciliation → explicitly qualified Chromium navigation/interaction/destruction/post-condition evidence.Buyer acceptance beyond this foundation remains #316 pending→accepted/quarantined integration, durable crash/process-restart recovery, real browser-observed destruction/post-conditions on an explicitly qualified Chromium lane, #299 3/3 replay, and protected-main release/SBOM/provenance/reproducibility/rollback.
No #229/main/#316 source mutation, force/destructive restack, self-approval, bypass, workflow/ruleset/secret change, sandbox weakening, provider/model pin, coverage weakening, merge, tag, or release.