fix: quarantine uncertain Browser Session creation - #315
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:
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 (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough브라우저 세션이 컨텍스트 생성·파괴 결과의 확실성을 추적합니다. 불확실한 결과는 Changes브라우저 세션 복구 격리
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant BrowserSession
participant DisposableContextPort
Caller->>BrowserSession: create disposable context
BrowserSession->>DisposableContextPort: create
DisposableContextPort-->>BrowserSession: uncertain failure or duplicate output
BrowserSession-->>Caller: creation error
BrowserSession->>BrowserSession: enter RecoveryRequired
Caller->>BrowserSession: request authority or normal end
BrowserSession-->>Caller: SessionNotActive
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 78.79% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 3 files. (3 skipped: 3 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 the exact current child head. This is a stacked causal-repair PR; verify the typed clean-vs-uncertain creation outcome, |
|
|
|
@coderabbitai review Please review exact current head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Exact HEAD and ancestry =='
git rev-parse HEAD
git status --short
git merge-base HEAD 6486e916dceb4ab5f33f7b390cd76fd4673d6007
git diff --stat 6486e916dceb4ab5f33f7b390cd76fd4673d6007..HEAD
git diff --name-status 6486e916dceb4ab5f33f7b390cd76fd4673d6007..HEAD
echo '== Changed-file outline =='
git diff --name-only 6486e916dceb4ab5f33f7b390cd76fd4673d6007..HEAD |
while IFS= read -r file; do
echo "--- $file"
ast-grep outline "$file" --items all 2>/dev/null || true
done
echo '== Relevant definitions and call sites =='
rg -n -C 4 --glob '*.rs' \
'BrowserSessionState|RecoveryRequired|CreateFailedClean|CreateFailedUncertain|createUserContext|create_user_context|DisposableIsolationId|destroy|duplicate' .
echo '== Documentation and contracts references =='
rg -n -C 3 \
'RecoveryRequired|CreateFailedClean|CreateFailedUncertain|Browser Session|duplicate.*(context|isolation)|DisposableIsolationId' \
. -g '!**/target/**' -g '!**/.git/**'
echo '== Current-head GitHub checks =='
gh api repos/ContextualWisdomLab/OriginWeave/commits/09a733a8609b54abfb7a4f6509b9eb9c064b9b2d/check-runs \
--paginate --jq '.check_runs[] | [.name,.status,.conclusion,.details_url] | `@tsv`' || true
gh pr view 315 --repo ContextualWisdomLab/OriginWeave \
--json headRefOid,statusCheckRollup,reviewDecision,isDraft \
--jq '{headRefOid,reviewDecision,isDraft,statusCheckRollup:[.statusCheckRollup[]|{name:.name,status:.status,conclusion:.conclusion,detailsUrl:.detailsUrl}]}' || trueLength of output: 196237 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Current-head immediate commit =='
git show --no-ext-diff --format=fuller --stat HEAD
git diff --no-ext-diff HEAD^..HEAD -- \
crates/originweave-browser-session/src/lib.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
echo '== Domain implementation, numbered =='
nl -ba crates/originweave-browser-session/src/lib.rs | sed -n '270,485p'
echo '== Domain tests, numbered =='
nl -ba crates/originweave-browser-session/src/lib.rs | sed -n '558,940p'
echo '== Repository contract, numbered =='
nl -ba tests/test_browser_session_lifecycle_contract.py | sed -n '1,130p'
echo '== ADR / traceability / UML, numbered =='
nl -ba docs/adr/0114-browser-session-disposable-context-authority.md | sed -n '1,130p'
nl -ba docs/traceability/browser-session-lifecycle-authority.md | sed -n '1,100p'
nl -ba docs/uml/browser-session-lifecycle-authority.md | sed -n '40,90p'
echo '== Focused source-level invariant checks =='
python3 - <<'PY'
from pathlib import Path
source = Path("crates/originweave-browser-session/src/lib.rs").read_text()
checks = {
"uncertain create enters recovery": "Err(DisposableContextPortError::CreateFailedUncertain) => {\n self.enter_recovery_required();",
"duplicate context enters recovery": "if self.contexts.contains_key(&handle.browsing_context) {\n self.enter_recovery_required();",
"duplicate isolation enters recovery": "if self\n .contexts\n .values()\n .any(|record| record.handle.isolation == handle.isolation)\n {\n self.enter_recovery_required();",
"destroy retains validated record": "let record = self.context_for_authority_mut(authority)?;\n let handle = record.handle.clone();\n match port.destroy_disposable_context(browser_session, &handle)",
"all active-only transitions use require_active": "self.require_active()?;",
}
for name, needle in checks.items():
print(f"{name}: {'PASS' if needle in source else 'FAIL'}")
PYLength of output: 62681
I found no blocking defect in the requested static-review scope.
I did not transfer CI evidence from You are interacting with an AI system. |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current lifecycle finding: destroy_disposable_context marks only the failed record Uncertain but leaves the BrowserSession aggregate Active. After an unproven destroy, the same session can still create another disposable boundary or mint/advance authority for unrelated active contexts, even though the aggregate has no recovery path for the uncertain record and normal end() is permanently blocked. That is weaker than the new RecoveryRequired invariant used for uncertain creation/duplicate outputs and weaker than #312's cleanup-failure fail-closed requirement. Required hostile RED: after DestroyFailed, state() must become RecoveryRequired; creating another context, issuing/advancing authority, and normal end must fail before adapter I/O. Minimal repair should route unproven destruction through the same aggregate recovery transition while preserving the exact failed handle as uncertain evidence. Do not weaken the port contract or treat an ACK as destruction proof.
|
@coderabbitai review Please review exact head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='f5780fb3102c35f4c0239696ab2499060fc9a55b'
base='6486e916dceb4ab5f33f7b390cd76fd4673d6007'
echo '== Exact head and scope =='
git rev-parse HEAD
git status --short
git merge-base "$head" "$base"
git diff --name-status "$base..$head"
git diff --stat "$base..$head"
git show --no-ext-diff --format=fuller --stat "$head"
echo '== Relevant changed-file outlines =='
git diff --name-only "$base..$head" |
while IFS= read -r file; do
case "$file" in
*.rs|*.py|*.md)
echo "--- $file"
ast-grep outline "$file" --items all 2>/dev/null || true
;;
esac
done
echo '== Browser Session implementation map =='
fd -a '^lib\.rs$' crates |
while IFS= read -r file; do
if rg -q 'RecoveryRequired|CreateFailedUncertain|DisposableContextHandle' "$file"; then
echo "--- $file"
rg -n -C 5 \
'enum BrowserSessionState|RecoveryRequired|CreateFailedClean|CreateFailedUncertain|enter_recovery_required|require_active|create_disposable_context|destroy_disposable_context|context_for_authority_mut|DisposableIsolationId' \
"$file"
fi
done
echo '== Hostile destroy-failure test and fixture changes =='
fd -a '^destroy_failure_requires_recovery\.rs$' . -E target |
while IFS= read -r file; do
echo "--- $file"
nl -ba "$file" | sed -n '1,280p'
done
echo '== Lifecycle repository contracts and documentation =='
fd -a '^test_browser_session_lifecycle_contract\.py$|^0114-browser-session-disposable-context-authority\.md$|^browser-session-lifecycle-authority\.md$' . -E target |
while IFS= read -r file; do
echo "--- $file"
rg -n -C 3 \
'RecoveryRequired|CreateFailedClean|CreateFailedUncertain|duplicate|destroy|DisposableIsolationId|cleanup|acknowledg' \
"$file"
done
echo '== Exact-head checks only =='
gh api "repos/ContextualWisdomLab/OriginWeave/commits/$head/check-runs?per_page=100" \
--jq '.check_runs[] | [.name,.status,.conclusion,.details_url] | `@tsv`'
gh pr view 315 --repo ContextualWisdomLab/OriginWeave \
--json headRefOid,reviewDecision,isDraft,statusCheckRollup \
--jq '{headRefOid,reviewDecision,isDraft,statusCheckRollup:[.statusCheckRollup[]|{name,status,conclusion,detailsUrl}]}'Length of output: 71398
I found no blocking defect in the requested static-review scope.
The lifecycle implementation remains fail closed.
ADR 0114, lifecycle traceability, UML, and repository contracts remain consistent with these rules. I did not use predecessor or parent CI as evidence. Exact-head run ✅ Action performedReview finished.
|
|
Exact-current DDD finding on |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current lifecycle finding on 98a28db0ac95972419906f229badf35c4f5fa0d6: record_transport_loss() cannot represent transport loss that happens after ownership has already entered RecoveryRequired. The method returns false for every non-Active state, and the current destroy-failure test explicitly expects false after RecoveryRequired. That makes a real new transport-loss event indistinguishable from an idempotent duplicate report. This matters for reconciliation: a recovery-required session with a still-live BiDi transport can later attest remote state, while the same ownership uncertainty followed by transport loss cannot safely use the same adapter/connection for reconciliation.
Required hostile RED: enter RecoveryRequired through an uncertain create or unproven destroy, then lose the browser transport. Preserve both facts without reopening authority; the first actual transport-loss report must be observable as a new event/state transition, subsequent duplicate reports must be idempotent, and no reconciliation/browser I/O that assumes the original transport remains live may be admitted. Minimal repair should model transport liveness orthogonally to ownership recovery (preferred) or use an explicit combined state/evidence rather than silently dropping the second failure. Do not simply overwrite RecoveryRequired with TransportLost, because that would erase the unresolved ownership/cleanup fact and any recovery identity added for review 5164398667.
This is separate from the already-known repository-contract literal RED and the lossless partial/duplicate-create recovery-identity finding. It should receive its own hostile test and fresh exact-head CI; predecessor coverage GREEN does not transfer.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-current authority finding on 98a28db0ac95972419906f229badf35c4f5fa0d6: the current anti-aliasing contract covers simultaneously live boundaries but still admits a sequential ABA/reincarnation alias. BrowserSession::start may reuse the same external BrowserSessionId and resets the local epoch to 1; PresentationMutationAuthority is identified by browser session + DisposableIsolationId + browsing context + local epoch. The port/ADR currently require DisposableIsolationId to be non-aliasing only for the live lifetime of one boundary, and the hostile two-aggregate test proves only the distinct-isolation case. After owner A destroys/ends its boundary, a later owner B can structurally reproduce the same (session, isolation, context, epoch) tuple if the remote/adapter reuses an isolation/user-context string after removal. In that case A's retained stale authority is indistinguishable from B's current authority and B.destroy_disposable_context(&authority_a, ...) reaches B's adapter I/O.
The 9 September WebDriver BiDi contract says a user context id is a unique string set on user-context creation and lookup/removal operate over the current set of user contexts; that is not a domain-level historical generation/non-reuse proof after removal. Do not strengthen OriginWeave's capability semantics by assuming an unstated lifetime guarantee.
Required hostile RED: A creates (S,U,C,epoch=1), proves destruction and ends; B then starts with the same S, adapter returns the same U and C, and B again reaches local epoch 1. B must reject A's retained authority before adapter I/O, while B's newly issued authority remains usable. Minimal repair must bind a non-reusable Browser Session/ownership incarnation (or equivalent verified browser-process/session generation) through both authority validation and lifecycle I/O/mapping. An aggregate-only nonce that the port ignores is insufficient, as ADR 0114 already correctly notes; an unchecked random adapter id is also insufficient. Keep this separate from the existing lossless recovery-evidence and RecoveryRequired → transport loss findings, and require a fresh exact-head CI after all three are repaired.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head RCA for 110eb33a6d368be977a0c37e49556af976ca09f6: CI 34470987805 is not source-wide RED. Rust contracts 102850580029 are GREEN; Production coverage 102850580204 reaches 100% functions/lines/branches but fails exact region coverage at 862/865 (99.6532%). Artifact 10149551238 (sha256:6078f1b1f935a7e1e18f5b796be029c6770b7b02654440bf30517c00fb76916e) identifies exactly three uncovered regions in crates/originweave-browser-session/src/lib.rs: line 304 BrowserSession::start's allocator error edge, line 435 advance_context_epoch's reserve_epoch()? overflow edge, and line 439 the second get_mut(...).ok_or(...) edge after the same key/state was already proven above.
Please repair causally rather than excluding coverage or adding a test-only production bypass. The epoch-overflow edge is genuinely reachable and should get a hostile unit case with an already-owned active context. The post-proof get_mut error edge is structurally unreachable in the current single-threaded &mut self flow; remove/restructure the redundant double lookup rather than manufacturing an impossible fixture. For the process-global incarnation exhaustion edge, keep fail-closed semantics but move allocation behind a private production helper that accepts the counter (or equivalent deterministic allocator boundary) so BrowserSession::start delegates without its own untestable error branch and the same production allocation path can be driven to exhaustion with an isolated counter. No coverage exclusion, sample reduction, or global-static mutation race.
The recovery-identity, transport-liveness, and sequential-ABA fixes visible in this head otherwise match the previously recorded invariants; this review does not transfer predecessor GREEN or authorize #313 adoption until a fresh exact head is 100% GREEN.
|
@coderabbitai review Please review exact head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head='0c6ed89c09ad16b5e170fd80604c29b16ce4f212'
predecessor='f5780fb3102c35f4c0239696ab2499060fc9a55b'
base='6486e916dceb4ab5f33f7b390cd76fd4673d6007'
repo='ContextualWisdomLab/OriginWeave'
echo '== Exact commit identity and scope =='
printf 'checked_out='
git rev-parse HEAD
printf 'requested='
git rev-parse "$head"
git merge-base "$head" "$base"
git status --short
git diff --name-status "$predecessor..$head"
git diff --stat "$predecessor..$head"
git show --no-ext-diff --format=fuller --stat "$head"
git diff --no-ext-diff --find-renames "$predecessor..$head"
echo '== Changed-file structure =='
git diff --name-only "$predecessor..$head" | while IFS= read -r file; do
echo "--- $file"
case "$file" in
*.rs|*.py) ast-grep outline "$file" --items all 2>/dev/null || true ;;
esac
done
echo '== Browser Session implementation map =='
fd -a '^lib\.rs$' crates -E target | while IFS= read -r file; do
if rg -q 'DisposableContextPort|ContextCreation|RecoveryRequired|epoch' "$file"; then
echo "--- $file"
rg -n -C 5 \
'enum (BrowserSessionState|BrowserSessionError|DisposableContextPortError)|trait DisposableContextPort|struct (DisposableContextHandle|OwnedContextRecord)|RecoveryRequired|TransportLost|incarnation|epoch|Epoch|create_disposable_context|destroy_disposable_context|context_for_authority_mut|enter_recovery_required|require_active' \
"$file"
fi
done
echo '== Related Rust tests and fixtures =='
rg -n -C 5 --glob '*.rs' \
'ABA|incarnation|epoch.*(exhaust|MAX)|exhaust.*epoch|TransportLost|transport.*liveness|RecoveryRequired|CreateFailed(Clean|Uncertain)|DestroyFailed|DisposableContextPort' \
crates -g '!**/target/**'
echo '== Contract and documentation evidence =='
fd -a '^(test_browser_session_lifecycle_contract\.py|0114-browser-session-disposable-context-authority\.md|browser-session-lifecycle-authority\.md)$' . -E target |
while IFS= read -r file; do
echo "--- $file"
rg -n -C 4 \
'incarnation|ABA|epoch|exhaust|RecoveryRequired|TransportLost|CreateFailedClean|CreateFailedUncertain|DestroyFailed|liveness|coverage' \
"$file"
done
echo '== Exact-head checks only =='
gh api "repos/$repo/commits/$head/check-runs?per_page=100" \
--paginate \
--jq '.check_runs[] | [.name,.head_sha,.status,.conclusion,.details_url] | `@tsv`'
gh run view 34475535820 --repo "$repo" --json databaseId,headSha,status,conclusion,event,workflowName,url,jobs \
--jq '{databaseId,headSha,status,conclusion,event,workflowName,url,jobs:[.jobs[]|{name,status,conclusion,url}]}'
gh pr view 315 --repo "$repo" --json headRefOid,isDraft,reviewDecision,statusCheckRollup \
--jq '{headRefOid,isDraft,reviewDecision,statusCheckRollup:[.statusCheckRollup[]|{name,headSha,status,conclusion,detailsUrl}]}'Length of output: 168311 🏁 Script executed: #!/bin/bash
set -euo pipefail
head='0c6ed89c09ad16b5e170fd80604c29b16ce4f212'
predecessor='f5780fb3102c35f4c0239696ab2499060fc9a55b'
echo '== Complete exact-head delta in reviewable sections =='
git diff --no-ext-diff --unified=80 "$predecessor..$head" -- \
crates/originweave-browser-session/src/lib.rs \
crates/originweave-browser-session/tests/destroy_failure_requires_recovery.rs \
crates/originweave-browser-session/tests/sequential_incarnation_reuse.rs \
tests/test_browser_session_lifecycle_contract.py
echo '== Exact lifecycle implementation ranges =='
nl -ba crates/originweave-browser-session/src/lib.rs | sed -n '1,240p'
nl -ba crates/originweave-browser-session/src/lib.rs | sed -n '288,585p'
nl -ba crates/originweave-browser-session/src/lib.rs | sed -n '718,1015p'
echo '== All DisposableContextPort implementations and API references =='
rg -n -U -C 4 --glob '*.rs' \
'(impl\s+DisposableContextPort\s+for|create_disposable_context\s*\(|destroy_disposable_context\s*\(|DisposableContextPortError|DisposableContextCreateError|DisposableContextDestroyError)' \
. -g '!**/target/**'
echo '== Documentation completeness and stale terminology =='
rg -n -i -C 3 \
'DisposableContextPortError|invalid_error_session|DestroyFailed.*create|CreateFailed.*destroy|RecoveryRequired.*TransportLost|TransportLost.*RecoveryRequired|BrowserSessionIncarnation|EpochExhausted|coverage' \
docs crates tests -g '!**/target/**'
echo '== Exact CI job step conclusions =='
gh run view 34475535820 --repo ContextualWisdomLab/OriginWeave --log-failed 2>&1 || true
gh api repos/ContextualWisdomLab/OriginWeave/actions/jobs/102865268437 \
--jq '{name,head_sha,conclusion,steps:[.steps[]|{name,status,conclusion,number}]}' || true
gh api repos/ContextualWisdomLab/OriginWeave/actions/jobs/102865268210 \
--jq '{name,head_sha,conclusion,steps:[.steps[]|{name,status,conclusion,number}]}' || trueLength of output: 202339
I found no blocking defect in the requested scope.
I verified exact-head CI run The PR remains Draft. I did not issue a formal approval. You are interacting with an AI system. |
bab489a
into
feat/browser-session-lifecycle-authority
1 participant
Scope
Causal repair child for #313 / #312. Base is
feat/browser-session-lifecycle-authorityat exact6486e916dceb4ab5f33f7b390cd76fd4673d6007; current head is exact0c6ed89c09ad16b5e170fd80604c29b16ce4f212. The parent branch remains untouched until this child has qualifying independent review and ordinary adoption evidence.Current Browser Session contract
This child now implements the Browser Session invariants previously found during review:
RecoveryRequired, which blocks further authority/lifecycle mutation while preserving evidence;RecoveryRequiredremains observable and duplicate loss reports are idempotent;BrowserSessionIncarnationis process-local and monotonic, participates inPresentationMutationAuthorityvalidation, and is passed through disposable-context create/destroy port calls so sequential reuse of external session/isolation/context/epoch identifiers cannot make a retained authority current again;Raw WebDriver/BiDi identifiers remain addressability, never policy authority.
RecoveryRequiredremains fail closed for normal authority issuance, epoch advance, destruction, and normal end until a separately authorized reconciliation path exists.Exact-head verification lineage
Exact
110eb33a6d368be977a0c37e49556af976ca09f6had repository contracts/tests/Clippy/rustdoc GREEN but exact production region coverage was 862/865 (99.6532%). Coverage artifact10149551238(sha256:6078f1b1f935a7e1e18f5b796be029c6770b7b02654440bf30517c00fb76916e) isolated three regions: incarnation-allocation exhaustion, active-context epoch exhaustion, and a structurally unreachable second context lookup.The repair moved incarnation allocation behind the same private production allocator path with an isolated counter for deterministic exhaustion testing, added the active-context epoch-exhaustion hostile case, and removed the redundant second lookup rather than fabricating an impossible fixture. Exact
6eacfe4876c369794927a904bbc7035dbc5712d3then exposed only a missing final newline through canonical rustfmt;3aa113b567624a96085b63cc657c9e3894f8ffa6repaired that formatting defect. Its coverage artifact10151172284(sha256:68dcb935983b12370db168aa044ff0230c8c0725626e0525c48df3724e8c5970) reduced the remaining gap to one test-macro region from the nonmatching branch ofmatches!, while functions/lines/branches were already 100%. Current0c6ed89...replaces that macro branch with an exactexpect_err+ equality assertion against the same production allocator path; no exclusion, denominator change, sample reduction, or global-static mutation was introduced.CI
34475535820is exact-current GREEN: Rust contracts job102865268437passes Python repository contracts, canonical formatting, locked Rust tests, strict Clippy, and rustdoc/API docs; Production coverage job102865268210passes measurement and exact function/line/region/branch enforcement.Adoption gate
This PR remains Draft until a qualifying independent reviewer evaluates the current exact head and any actionable threads are settled. Earlier CodeRabbit output covered only through
f5780fb3102c35f4c0239696ab2499060fc9a55band does not qualify as current-head review. Do not self-approve or transfer predecessor review evidence.After current-head independent review, ordinary adoption into #313 must be followed by fresh verification of the resulting #313 exact head. Browser Session → BiDi ACL projection remains owned by #314 and must consume live aggregate authority rather than raw IDs or a retained token. Real Chromium
browser.createUserContext/browsingContext.create/browser.removeUserContextintegration, browser-observed destruction, #299 historical replay, and a separate current-Stable Chromium qualification remain later buyer acceptance under #292.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 repair.