Skip to content

fix(browser-session): require aggregate-issued lifecycle request authority - #317

Draft
seonghobae wants to merge 46 commits into
feat/privacy-presentation-identityfrom
fix/browser-session-lifecycle-request-capability
Draft

fix(browser-session): require aggregate-issued lifecycle request authority#317
seonghobae wants to merge 46 commits into
feat/privacy-presentation-identityfrom
fix/browser-session-lifecycle-request-capability

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

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:

  • review 5175575251: RecoveryRequired now projects each indirectly invalidated Active sibling exactly once as non-authorizing RecoveryRequiredOwnedHandle, while preserving the triggering handle's cause-specific evidence without duplication. External hostile fixture recovery_required_sibling_evidence.rs proves the two-context destruction-failure case.
  • review 5175813759: BoundBrowserSession::finish(&mut self) validates completion without consuming the wrapper on rejection. ActiveContextRemains therefore retains the exact bound adapter and ownership ledger so the caller can destroy/reconcile through the same owner and retry. External hostile fixture bound_session_abandonment.rs proves reject → same-owner cleanup → successful retry with no abandonment signal.
  • review 5176486914 remains a source blocker on this exact head: create-related recovery evidence (DuplicateAdapterHandle / UnsettledAdapterHandle) retains only the returned handle and loses the aggregate-issued attempt_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.
  • review 5176783252 is a standards-provenance blocker: the canonical W3C TR/webdriver-bidi currently 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.
  • repository contracts, ADR 0114, traceability, UML, and docs/product-technical-gap-baseline.md must be synchronized with the recovery-correlation and standards-provenance repairs. The current baseline continuity note predates review 5176486914 and must not describe this PR as awaiting verification only.
  • hosted CI 34578759212 on this exact head is terminal RED. Repository contracts are 176/176 GREEN, cargo fmt --all --check is GREEN, and cargo test --locked --workspace --all-targets is GREEN. Strict Clippy fails at bind_lifecycle_port with clippy::double_must_use because the function has a bare #[must_use] while BoundBrowserSession<P> is already #[must_use]; the duplicate-handle fixture also has unnecessary mut. Remove the redundant annotation/dead mutability rather than allowing the lint. Rustdoc/API docs did not run.
  • the same CI's production coverage artifact 10190957720 (sha256:a6757604038ab58d7d4d18a359f5e3d5d1c6495ddd40d37586298ccbe59eb155) reports branches 800/800, functions 699/699, lines 5920/5924, regions 7314/7318. Remaining uncovered enter_recovery_required() branches intersect review 5176486914; repair the recovery semantics rather than adding coverage-only branches or exclusions.
  • hosted CI 34577862599 on predecessor bd1bd857bba754ed06e3c326eeeba953f96faaa0 passed Python repository contracts but failed cargo fmt --all --check. Its uploaded rustfmt artifact 10190470645 identified only src/lib.rs, bound_session_abandonment.rs, and recovery_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.
  • historical exact d5046e76cb7555b448b728ea1bed9ba1ea8de8c3 / CI 34573175780 likewise 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.
  • Browser Session reserves BrowserContextEpoch before remote create and carries it in opaque DisposableContextCreateRequest; aggregate-issued DisposableContextCreateCompletion settles exact Accepted/Rejected candidates.
  • AuthorizedContextOperationRequest<O> has no public constructor. AuthorizedContextOperationPort keeps operation semantics adapter-owned while Browser Session validates current PresentationMutationAuthority before I/O and routes only to the same consumed adapter.
  • BoundBrowserSession<P> manual Debug never invokes P::fmt and redacts the adapter.
  • transport loss preserves exact active handles as non-authorizing TransportLossOwnedHandle recovery evidence and is idempotent.
  • unresolved wrapper Drop performs no browser I/O and only increments a process-local abandonment signal. That signal is neither exact recovery evidence nor destruction proof.
  • protocol-specific WebDriver BiDi tuple contents, pending/accepted/quarantined storage, command semantics, and remote-liveness reconciliation remain [Browser Session/BiDi ACL] Bind current lifecycle authority to presentation command planning #314/feat(bidi): bind current Browser Session authority to BiDi planning #316-owned.

Current Browser Session merge blockers are source repair plus exact verification: repair review 5176486914 with aggregate-issued create-attempt correlation while preserving previously accepted same-valued ownership as a distinct recovery fact; repair standards trace per 5176783252; 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. Review 5174248631 is covered only if the current failed-finish retention evidence passes; no historical review is promoted across heads. Historical review 5174624587 remains retracted by 5175126760.

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.

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: d786d13b-2d0e-4bcf-b949-eeb64ea36473

📥 Commits

Reviewing files that changed from the base of the PR and between 6d87dff and 9cde981.

📒 Files selected for processing (10)
  • crates/originweave-browser-session/src/lib.rs
  • crates/originweave-browser-session/tests/destroy_failure_requires_recovery.rs
  • 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
  • docs/adr/0114-browser-session-disposable-context-authority.md
  • docs/traceability/browser-session-lifecycle-authority.md
  • docs/uml/browser-session-lifecycle-authority.md
  • tests/test_browser_session_lifecycle_contract.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

BrowserSession이 구체적인 포트를 소비하는 BoundBrowserSession을 제공합니다. 생성·삭제 요청은 aggregate 내부에서 opaque 타입으로 발행됩니다. 포트 계약, 테스트, 권한 문서와 UML이 새 호출 흐름에 맞게 갱신되었습니다.

Changes

라이프사이클 포트 권한

Layer / File(s) Summary
요청 계약과 바운드 세션 API
crates/originweave-browser-session/src/lib.rs
DisposableContextCreateRequestDisposableContextDestroyRequest를 추가했습니다. DisposableContextPort는 요청 객체를 받습니다. bind_lifecycle_port는 aggregate와 구체 포트를 BoundBrowserSession으로 결합합니다.
구현 상태와 포트 호출 검증
crates/originweave-browser-session/src/lib.rs, crates/originweave-browser-session/tests/*
내부 테스트 포트와 통합 테스트를 새 API로 변경했습니다. 바운드 포트만 생성·삭제 호출을 받으며, 바인딩 중 adapter 콜백은 실행되지 않고, 다른 포트로 교체할 수 없음을 검증합니다. 순차 incarnation과 삭제 실패 복구 검사도 갱신했습니다.
권한 설계 문서 갱신
docs/adr/0114-browser-session-disposable-context-authority.md
선형 lifecycle-port binding, opaque 요청, 콜백 금지, 대체 포트 차단 및 마이그레이션 내용을 반영했습니다.
추적성, UML, 계약 테스트 갱신
docs/traceability/browser-session-lifecycle-authority.md, docs/uml/browser-session-lifecycle-authority.md, tests/test_browser_session_lifecycle_contract.py
추적성 문서와 UML을 새 호출 흐름으로 변경했습니다. 계약 테스트는 새 공개 타입과 메서드, 제거된 API, 생성자 비공개 상태를 검사합니다.

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)
Loading

Merge Risk: ⚪ Minimal · up to 9cde9

No actionable current-head defect remains; complete the normal required checks before merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 aggregate-issued lifecycle request authority를 요구하도록 Browser Session lifecycle 권한을 변경한 핵심 내용을 정확하고 간결하게 설명합니다.
Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/browser-session-lifecycle-request-capability

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae marked this pull request as draft September 10, 2026 19:22
@seonghobae
seonghobae marked this pull request as ready for review September 10, 2026 19:27
@seonghobae
seonghobae marked this pull request as draft September 10, 2026 19:33

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@seonghobae
seonghobae marked this pull request as ready for review September 10, 2026 19:37
@seonghobae
seonghobae marked this pull request as draft September 10, 2026 19:47

Copy link
Copy Markdown
Contributor Author

Exact-head RED evidence for 9caf9bbe4228c443b7d5a4279831765a6a38765a is now causal and reproducible. CI 34522121442 reached the Browser Session integration suite after repairing the pre-existing lifecycle-port test-signature drift. All earlier Browser Session unit/integration cases passed; only the new same-id/different-adapter hostile tests failed.

Coverage job 103021942177 observed:

  • distinct_port_with_same_claimed_id_cannot_create: actual Ok(PresentationMutationAuthority { browser_session: 17, isolation: "spoofed-isolation", browsing_context: 42, context_epoch: 2, ... }), expected Err(LifecyclePortMismatch).
  • distinct_port_with_same_claimed_id_cannot_destroy: actual Ok(()), expected Err(LifecyclePortMismatch).

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 + DisposableContextPortId binds lifecycle mutation to one adapter instance. Keep this RED until the port ownership is non-forgeable/non-replayable; do not weaken or delete the hostile fixture.

Repository status on this exact head is independently RED at cargo fmt --check as well. Rustfmt diagnostics artifact 10170079709, digest sha256:982aee39ad1383379288ff82575a950d0d72cbee6b2c28b8de62d15d10734068, remains available. Production coverage cannot proceed to exact enforcement while the intentional hostile tests fail. A separate nightly diagnostic also flags AtomicU64::fetch_update as deprecated in favor of try_update; treat that as root-fix work only after compatibility with the pinned stable toolchain is verified, not as a warning suppression exercise.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@seonghobae
seonghobae marked this pull request as ready for review September 10, 2026 20:09
@seonghobae
seonghobae marked this pull request as draft September 10, 2026 20:13
@seonghobae
seonghobae marked this pull request as ready for review September 10, 2026 21:14

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Please review exact head 9cde981899950b900698a17e7fa739af59f6bb4f, especially the security/API claim that consuming BrowserSession + P into BoundBrowserSession<P> removes pre-authority adapter callbacks and ordinary distinct-adapter substitution/replay without moving protocol-specific BiDi ownership into the Browser Session domain.

@coderabbitai

coderabbitai Bot commented Sep 10, 2026

Copy link
Copy Markdown
🧩 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
done

Length 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"
done

Length 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


@seonghobae Review complete for exact head 9cde981899950b900698a17e7fa739af59f6bb4f.

I found no code-level issue in the requested security/API change.

  • BrowserSession::bind_lifecycle_port(self, port) only moves the aggregate and port into private BoundBrowserSession<P> fields. It invokes no adapter callback.
  • The public lifecycle methods exist on BoundBrowserSession<P>. They accept no replacement port.
  • Repository-wide inspection found no DisposableContextPortId, port_id(), mutable port accessor, extraction method, or alternate public lifecycle entry point.
  • Create validates Active state and reserves the epoch before it calls the bound port.
  • Destroy validates state, session, incarnation, isolation, context, and epoch before it calls the bound port.
  • The request types have private fields and no public constructors.
  • The external hostile fixtures cover the pre-authority callback case, ordinary distinct-adapter substitution, sequential-incarnation replay, and recovery behavior.
  • originweave-browser-session depends only on originweave-core. It does not import or depend on the BiDi crate. The BiDi-specific pending → accepted/quarantined state and remote-liveness responsibility remain outside the Browser Session domain.

The structural binding prevents ordinary callers from substituting or replaying a different adapter after binding. It cannot prevent a malicious trusted DisposableContextPort implementation from internally forwarding an authorized call. ADR 0114 states this TCB boundary.

Exact-head verification is not complete. GitHub reports Rust contracts and Production coverage as queued, and the PR state is UNSTABLE. I cannot determine readiness or approve the PR until the required exact-head checks are terminal and GREEN.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
seonghobae marked this pull request as ready for review September 11, 2026 07:04
@seonghobae
seonghobae marked this pull request as draft September 11, 2026 08:04
@seonghobae
seonghobae marked this pull request as ready for review September 11, 2026 08:10
@seonghobae
seonghobae marked this pull request as draft September 11, 2026 08:16
@seonghobae
seonghobae marked this pull request as ready for review September 11, 2026 08:21

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@seonghobae
seonghobae marked this pull request as draft September 11, 2026 08:34

Copy link
Copy Markdown
Contributor Author

Exact-head CI 34578759212 RCA on 56a96ad8407b418d1775cfdf091519b57ad1e893 is now terminal and code-caused, not runner-caused.

Rust contracts 103197204149: repository contracts 176/176 GREEN, cargo fmt --all --check GREEN, and cargo test --locked --workspace --all-targets GREEN. Strict Clippy then fails at crates/originweave-browser-session/src/lib.rs:583 with clippy::double_must_use: bind_lifecycle_port(...) -> BoundBrowserSession<P> has a bare #[must_use] even though BoundBrowserSession<P> is itself #[must_use]. Under -D warnings this is the exact gate failure; rustdoc/API docs never ran. Minimal fix is not a lint allow: remove the redundant function-level attribute, or give it a genuinely additional contract message only if one exists. The same compilation also reports an unnecessary mut in the duplicate-handle test fixture at line 1347; remove the dead mutability rather than tolerate it.

Production coverage 103197204361: artifact 10190957720, digest sha256:a6757604038ab58d7d4d18a359f5e3d5d1c6495ddd40d37586298ccbe59eb155. Exact totals are branches 800/800, functions 699/699, lines 5920/5924, regions 7314/7318. The only zero-execution source arms in originweave-browser-session/src/lib.rs are inside enter_recovery_required() at the PartialCreationIsolation(_) => false arm and the RecoveryRequiredOwnedHandle | TransportLossOwnedHandle equality arm. This is not a reason to add synthetic coverage: it intersects review 5176486914. The current value-level dedup mixes create-candidate/recovery causes with prior accepted ownership. The causal repair should narrow dedup to semantically identical recovery facts and add the create attempt correlation required by that review; then tests should naturally cover the remaining branches.

Nightly coverage compilation additionally warns that AtomicU64::fetch_update is renamed to try_update on that nightly. Do not suppress or blindly switch while the canonical stable toolchain is Rust 1.97.1; resolve it through a compatibility-proven toolchain/API root fix, not a warning allow.

I returned #317 to Draft because the exact head is RED and review 5176486914 is a real source blocker. No rerun is useful until the source writer produces a successor head.

Copy link
Copy Markdown
Contributor Author

docs/product-technical-gap-baseline.md is no longer fully current after review 5176486914. Its 2026-09-11 continuity note accurately records the existing Browser Session repairs, but it still frames #317 as awaiting verification only and does not record the newly proven create-recovery transaction-correlation gap. On the successor source fix, update that note together with ADR 0114/traceability: active-PR truth must state that create-related recovery evidence must preserve aggregate-issued attempt identity separately from any previously accepted same-valued handle, and that #317 remains source-blocked until the hostile duplicate-H/rejected-completion-failure case is GREEN. Keep protected-main truth separate; do not promote the active PR capability as shipped behavior.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant