feat: add Browser Session lifecycle authority - #313
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
Changes브라우저 세션 수명주기
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant BrowserSession
participant DisposableContextPort
Caller->>BrowserSession: create_disposable_context
BrowserSession->>DisposableContextPort: create_disposable_context
DisposableContextPort-->>BrowserSession: DisposableContextHandle
BrowserSession-->>Caller: PresentationMutationAuthority
Caller->>BrowserSession: destroy_disposable_context(authority)
BrowserSession->>DisposableContextPort: destroy_disposable_context
DisposableContextPort-->>BrowserSession: destruction result
Caller->>BrowserSession: end
Merge Risk: 🟡 Moderate · up to Duplicate adapter output can leave a browser isolation boundary untracked while the session reports normal termination. This lifecycle leak should be fixed before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 76.74% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 43 functions across 3 files. (8 skipped: 8 unsupported.) ✨ Finishing Touches 💡 1📝 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 |
|
@coderabbitai review Please review exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
target='9146d62aa71a8e480dd6ec58258e2217c7d0b293'
base='7ec83c1be1a8e8724d37c2d6ebbdb215b1b10e23'
echo '== Exact head verification =='
git rev-parse HEAD
git cat-file -t "$target"
git show -s --format='commit=%H%nparents=%P%nsubject=%s' "$target"
echo '== Commit range and changed files =='
git diff --name-status "$base" "$target"
git diff --stat "$base" "$target"
echo '== Workspace and CI candidates =='
fd -HI -t f '(^package\.json$|^pnpm-workspace\.yaml$|^turbo\.json$|^nx\.json$|^.*\.(ya?ml|json|ts|tsx|js|mjs|cjs)$)' . \
| rg -n '(^|/)(package\.json|pnpm-workspace\.yaml|turbo\.json|nx\.json)$|\.github/workflows/|browser|session' \
| head -300
echo '== Changed-file outlines =='
git diff --name-only "$base" "$target" | while IFS= read -r f; do
echo "--- $f"
case "$f" in
*.ts|*.tsx|*.js|*.mjs|*.cjs)
ast-grep outline "$f" --items all --view expanded 2>&1 || true
;;
*)
sed -n '1,260p' "$f"
;;
esac
done
echo '== Full target diff =='
git diff --find-renames --find-copies --unified=80 "$base" "$target"
echo '== Authority and lifecycle references across repository =='
rg -n -i -C 4 \
'DisposableContextPort|BrowserSession|browsing.?context|user.?context|epoch|quarantine|transport.?loss|destroy|dispose|WebDriver|BiDi|CDP' \
-g '!node_modules/**' -g '!dist/**' -g '!build/**' . | head -1200Length of output: 201886 🏁 Script executed: #!/bin/bash
set -euo pipefail
target='9146d62aa71a8e480dd6ec58258e2217c7d0b293'
base='7ec83c1be1a8e8724d37c2d6ebbdb215b1b10e23'
git checkout --detach "$target" >/dev/null 2>&1 || true
echo '== Exact head and changed paths =='
printf 'HEAD='; git rev-parse HEAD
git diff --name-status "$base" "$target"
echo
echo '== Browser Session source with line numbers =='
nl -ba crates/originweave-browser-session/src/lib.rs | sed -n '1,340p'
echo
echo '== Core identity definitions =='
rg -n -C 8 'struct BrowserSessionId|impl BrowserSessionId|struct BrowsingContextId|impl BrowsingContextId' \
crates/originweave-core
echo
echo '== BrowserSession construction and authority call sites =='
rg -n -C 5 'BrowserSession::start\s*\(|BrowserSession\s*\{|PresentationMutationAuthority\s*\{' \
-g '*.rs' -g '*.py' -g '*.md' . \
| head -700
echo
echo '== Documentation navigation entries =='
nl -ba docs/README.md | sed -n '82,108p'
echo
rg -n -C 3 '0114-browser-session|Browser Session lifecycle authority' docs README.md ARCHITECTURE.md CHANGELOG.md
echo
echo '== PR `#313` checks and metadata (informational only) =='
gh pr view 313 --json headRefOid,baseRefOid,statusCheckRollup,reviewDecision,isDraft,title,url \
--jq '{headRefOid,baseRefOid,reviewDecision,isDraft,title,url,statusCheckRollup:[.statusCheckRollup[]? | {name,conclusion,status,detailsUrl}]}'Length of output: 41064
Blocking finding — Browser Session identity can alias across aggregates
If two aggregates use the same Make aggregate identity non-aliased for a browser session. For example, make lifecycle construction consume a non-duplicable session lease from the owner, or include an aggregate-incarnation value in The remaining reviewed properties are correctly fail-closed within one aggregate: raw context lookup does not mint authority; stale and numerically foreign authority is rejected; destruction failure and transport loss quarantine authority; and The BiDi integration and real-browser cleanup evidence remain explicitly deferred. The exact-head checks are queued, so I did not treat CI as passing evidence.
|
|
Exact-current reviewer handoff on Preserve this as an executable hostile RED before repair: start two aggregates with the same Important acceptance nuance: adding an incarnation field only to The active #312/#313 writer lease remains authoritative; this is review evidence only, not a competing source write. |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head RCA confirms two independent blockers on 797157a. Hosted Rust-contract job 102744511014 executed and failed only because docs/README.md omits ADR 0114 from the canonical ADR index (175 Python tests, exactly one failure). Coverage job 102744510755 executed; functions/lines/branches are 100%, but regions are 485/487 (99.5893%), with the remaining inactive-session error regions in presentation_authority and advance_context_epoch. The security finding from review 5613165179 remains valid: BrowserSessionId+browsing-context+local epoch can alias across aggregate incarnations, and destroy I/O is still scoped only by those aliasable identifiers. I am repairing this by carrying a non-aliasing disposable isolation identity from the browser lifecycle port through stored context, mutation authority, pre-I/O validation, and destruction. For WebDriver BiDi the adapter proof obligation is a one-to-one mapping to the spec-defined unique user-context id; raw context/session identifiers remain addressability, not authority. Hostile acceptance must cover two aggregates with the same external session/context IDs and prove A's authority cannot trigger B I/O while B's authority destroys only B's isolation boundary. No gate weakening, force update, or mutable cross-repo dependency is part of the fix.
…recovery-required fix: quarantine uncertain Browser Session creation
|
@coderabbitai review Please review exact parent head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
target='bab489ab244e0edfdb309692ee1f9ecd2e7adde0'
base='7ec83c1be1a8e8724d37c2d6ebbdb215b1b10e23'
echo '== Exact target and topology =='
git cat-file -t "$target"
git show -s --format='commit=%H%nparents=%P%nsubject=%s%nbody=%B' "$target"
git merge-base --is-ancestor "$base" "$target" && echo "base_is_ancestor=true"
echo '== Commits from stacked base =='
git log --oneline --decorate "$base..$target" --max-count=80
echo '== Files changed from stacked base =='
git diff --name-status "$base" "$target"
echo '== Browser Session source outline =='
ast-grep outline crates/originweave-browser-session/src/lib.rs --items all --view expanded
echo '== Browser Session implementation =='
git show "${target}:crates/originweave-browser-session/src/lib.rs" | nl -ba | sed -n '1,680p'
echo '== Browser Session tests and contract tests =='
git ls-tree -r --name-only "$target" | rg '(^|/)(test_.*browser.*session.*\.py|.*browser.*session.*test.*|lib\.rs)$' \
| while IFS= read -r f; do
case "$f" in
crates/originweave-browser-session/src/lib.rs|*browser*session*)
echo "--- $f"
git show "${target}:${f}" | nl -ba | sed -n '1,760p'
;;
esac
done
echo '== Recovery, transport, incarnation, ABA, epoch, and port references =='
git grep -n -I -E \
'RecoveryRequired|OwnershipUncertain|TransportLost|incarnation|ABA|epoch|EpochExhausted|DisposableContextPort|DisposableContextHandle|destroy_disposable_context|create_disposable_context|cleanup|rollback|duplicate' \
"$target" -- '*.rs' '*.py' '*.md' | head -1800
echo '== Target changes relevant to this review =='
git diff --find-renames --unified=80 "$base" "$target" -- \
crates/originweave-browser-session/src/lib.rs \
tests \
docs \
ARCHITECTURE.md Cargo.toml Cargo.lock | head -3000
echo '== Hosted PR exact-head checks =='
gh pr view 313 --repo ContextualWisdomLab/OriginWeave \
--json headRefOid,baseRefOid,isDraft,reviewDecision,statusCheckRollup,url \
--jq '{headRefOid,baseRefOid,isDraft,reviewDecision,url,statusCheckRollup:[.statusCheckRollup[]? | {name,status,conclusion,detailsUrl}]}'Length of output: 181839 🏁 Script executed: #!/bin/bash
set -euo pipefail
target='bab489ab244e0edfdb309692ee1f9ecd2e7adde0'
echo '== Target topology and recent commits =='
git show -s --format='commit=%H%nparents=%P%nsubject=%s' "$target"
git log --graph --oneline --decorate -25 "$target"
echo '== Browser Session crate manifest and all standalone Rust tests =='
git show "${target}:crates/originweave-browser-session/Cargo.toml" | nl -ba
git ls-tree -r --name-only "$target" crates/originweave-browser-session/tests \
| while IFS= read -r f; do
echo "--- $f"
git show "${target}:${f}" | nl -ba | sed -n '1,520p'
done
echo '== Focused source: public API and state transition methods =='
git show "${target}:crates/originweave-browser-session/src/lib.rs" | nl -ba \
| sed -n '1,460p'
echo '== Parent-only commits after `#315` repair tip candidates =='
git log --format='%H %P %s' --ancestry-path 6486e916dceb4ab5f33f7b390cd76fd4673d6007.."$target" \
| head -80
echo '== Changed files in each recent target ancestor =='
git log --format='%H' -12 "$target" | while IFS= read -r c; do
echo "--- $c $(git show -s --format=%s "$c")"
git diff-tree --no-commit-id --name-status -r "$c" | head -120
done
echo '== Public uses and implementations outside the crate =='
git grep -n -I -E \
'BrowserSession::start|impl[[:space:]]+DisposableContextPort|PresentationMutationAuthority|BrowserSessionRecoveryEvidence|BrowserSessionIncarnation' \
"$target" -- ':!crates/originweave-browser-session/src/lib.rs' \
| head -1000Length of output: 53655
No blocking finding in the requested Browser Session scope.
Hosted exact-head evidence matches the stated status. Rust contracts and Production coverage are successful on run You are interacting with an AI system. |
6d87dff
into
feat/privacy-presentation-identity
1 participant
Scope
Implements #312 as the Browser Session bounded-context foundation stacked on #229 exact
7ec83c1be1a8e8724d37c2d6ebbdb215b1b10e23. Browser-domain truth remains in OriginWeave; this PR does not copy browser/sandbox/workflow authority from #148/#212/.github or turn WebDriver/BiDi identifiers into policy authority.Current head is exact
bab489ab244e0edfdb309692ee1f9ecd2e7adde0, produced by the ordinary merge of reviewed child #315 exact0c6ed89c09ad16b5e170fd80604c29b16ce4f212into this branch. No force push or destructive restack was used.Domain contract
originweave-browser-sessionowns the Browser Session aggregate and disposable-context lifecycle boundary.DisposableContextPortreturns the exact browser-issued isolation/user-context identity together with its browsing-context address; these values are lifecycle addressability, not authority.PresentationMutationAuthoritybinds Browser Session transport identity, process-local monotonicBrowserSessionIncarnation, isolation identity, browsing context, and context epoch.UnprovenDestructionand entersRecoveryRequired; no command acknowledgement is treated as destruction proof.RecoveryRequiredremains fail closed for normal authority issuance, context creation, epoch advance, destruction, and normal end.The lifecycle adapter must eventually map this contract to real
browser.createUserContext/browsingContext.create/browser.removeUserContextbehavior and prove browser-observed destruction. That transport work is intentionally not implemented here. #314 owns the next Browser Session → BiDi ACL projection and may only consume live aggregate authority after this foundation is verified.Verification lineage
The pre-adoption #313 head
6486e916dceb4ab5f33f7b390cd76fd4673d6007was RED: CI34440868057failed canonical formatting and exact production coverage. Child #315 repaired the later review findings and exact coverage gaps causally, then reached exact-current GREEN at0c6ed89...in CI34475535820: Rust contracts102865268437passed repository contracts, formatting, locked tests, strict Clippy, and rustdoc/API docs; Production coverage102865268210passed exact function/line/region/branch enforcement. Coverage artifact10151357764is bound to that child exact head with digestsha256:fef75f5a1c604731c5a8a62d0a5ced3e6256a070552b77079af88c51264600b0.CodeRabbit independently reviewed exact child head
0c6ed89...after the final coverage repair and reported no blocking defect in the requested lifecycle scope. That review was not a formal GitHub approval and is not transferred as parent-head verification.The merge-generated parent CI
34476134476forbab489ab...skipped both jobs because this parent was Draft, so it is not parent GREEN. This PR must run a fresh exact-head repository generation before any further adoption.Gate
Keep this PR unmerged until current
bab489ab...has exact-head repository contracts, canonical formatting, locked tests, strict Clippy, rustdoc, exact function/line/region/branch coverage, independent current-head review/thread settlement, and any required central control checks for its target path. Earlier child/predecessor GREEN is evidence of lineage only and does not replace parent verification.Real Chromium user-context creation/removal, destruction post-condition, #299 historical Chrome 150 replay, and a separate current-Stable Chromium qualification remain buyer acceptance under #292. Historical #299 evidence must not be repinned.
No force push, destructive rebase, self-approval, protection bypass, workflow/ruleset/secret mutation, provider/model pin, sandbox weakening,
--no-sandbox, protected-main merge, tag, or release is part of this foundation step.