fix(browser): prove PID-safe browser crash recovery evidence - #148
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headcb49d4c6c26b32e2bdab5634622624b47a6ca8f3. -
Head SHA:
cb49d4c6c26b32e2bdab5634622624b47a6ca8f3 -
Workflow run: 32237343063
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["CI script: run_mv3_compatibility.py"]
S1 --> I1["review and security gate shell path"]
I1 --> R1["Review risk: CI script: run_mv3_compatibility.py"]
R1 --> V1["bash -n plus Strix self-test"]
Evidence --> S2["Test: test_agent_task_browser_crash_recovery_contract.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_agent_task_browser_crash_recovery_contract.py"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["CI script: run_mv3_compatibility.py"]
S1 --> I1["review and security gate shell path"]
I1 --> R1["Review risk: CI script: run_mv3_compatibility.py"]
R1 --> V1["bash -n plus Strix self-test"]
Evidence --> S2["Test: test_agent_task_browser_crash_recovery_contract.py"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_agent_task_browser_crash_recovery_contract.py"]
R2 --> V2["targeted test run"]
|
seonghobae
left a comment
There was a problem hiding this comment.
Current-head operability finding on 0135984f1bc1f68d89d7777f49c4999474105a12, using the already-executed real-browser RED rather than another blind rerun.
MV3 artifact 9977680352 records Chrome/ChromeDriver 150.0.7871.129, all four sandbox-enabled browser lanes at 0/3, and browser-crash trials at failure_stage=session_create with only failure_type=RuntimeError / reason_code=runtime_error. That evidence proves fail-closed startup failure, but it cannot distinguish the causal classes #212 must choose between.
The current runner makes that ambiguity structural: every ChromeDriver process sends stdout/stderr to DEVNULL, while _json_request parses a WebDriver error payload only to special-case no such window; every other value.error/message, including session not created, is collapsed to generic RuntimeError and the response details are discarded. The crash classifier therefore cannot produce more specific startup evidence even when ChromeDriver supplied it. Re-running the unchanged workflow cannot improve the RCA.
Preserve the no---no-sandbox boundary. Before another hosted browser rerun, add the smallest credential-safe diagnostic boundary: retain the bounded WebDriver error code as a typed value (at minimum session not created) and classify only an allowlisted set of startup causes from bounded ChromeDriver diagnostics into closed reason codes, with raw response/log text, profile paths, page data and credentials excluded from the artifact. A focused contract should prove that a representative session-creation error produces the safe typed reason while arbitrary/unrecognized text remains session_not_created/unknown and is never emitted. Then the next real pinned-Chromium run can distinguish a sandbox/helper failure from a different browser-startup defect and hand the exact cause to #212 instead of treating the current coarse category as root cause.
This is diagnostic/operability authority only. It must not add a fallback launch, restore --no-sandbox, weaken the 3-trial denominator, or turn cleanup success into browser success.
|
Exact-head diagnostic repair landed as The prior real-browser failure remains the executed RED: Manifest V3 run This repair keeps the sandbox and all repeatability/cleanup gates unchanged. It adds a typed, redacted TDD evidence:
Successor runs are newly materialized and remain non-terminal:
Keep Draft. These queued runs are not passing evidence; the next real pinned-Chromium artifact must establish the allowlisted startup cause before #212 chooses any workflow-side helper repair. |
|
Exact-head RCA update — PR #148 @ Manifest V3 Compatibility run The paired causal boundary is unchanged and stronger than a blind retry: current #148 fails closed without sandbox-disabling arguments, while #43 @ |
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head RCA extension on 2323ff54e7bcad4c37eddfeb9274562982d52ac5: the bounded session_not_created repair is working, but the current runner still cannot refine startup_reason=unknown from ChromeDriver process diagnostics. Both _run_browser_pass and _run_agent_task_browser_pass launch ChromeDriver with stdout=subprocess.DEVNULL and stderr=subprocess.STDOUT; _classify_webdriver_session_startup_reason therefore sees only the W3C response value.message. The exact hosted artifact from MV3 run 34232690664 remains 0/3 at session creation with _WebDriverSessionNotCreatedError / session_not_created / startup_reason=unknown. Re-running the unchanged head cannot recover process-output evidence that this code discards.
Treat this as a source-side diagnostic RED distinct from #212's workflow-helper authority. The smallest safe repair should stream/drain ChromeDriver diagnostics through a bounded closed-vocabulary classifier while retaining no raw diagnostic bytes, paths, profile names, capabilities, page data, or secret-shaped text. The regression should include the sandbox marker split across read chunks plus hostile path/secret markers and prove that only reviewed reason codes survive; unrecognized diagnostics remain unknown. The capture must not introduce a PIPE deadlock, retry, alternate launch, broader egress, or --no-sandbox fallback. After exact repository GREEN, rerun pinned Chrome/ChromeDriver 150.0.7871.129; only then may a classified helper/AppArmor diagnostic be used as causal evidence for #212. Until then, keep the helper repair technically plausible but not proven by #148's current unknown evidence.
|
Writer lease ACTIVE — exact |
|
Writer lease RELEASED — current exact |
|
Writer lease ACTIVE — exact |
seonghobae
left a comment
There was a problem hiding this comment.
Current exact-head RCA for CI 34256771021: the bounded process-diagnostic production repair is present, but the two follow-up contracts added on 2b8e0e2... and 0e320aa... are not yet implemented. tests/test_chromedriver_process_diagnostic_contract.py now requires (1) the shared launch argv to include --verbose without --log-path, and (2) _drain_chromedriver_diagnostics to accept existing text-mode process doubles while retaining no raw diagnostic state. At 0e320aa..., _start_chromedriver still launches only [chromedriver, --port, --allowed-ips], and _drain_chromedriver_diagnostics raises TypeError for every non-bytes chunk. Therefore the Python repository-contract failure is causal and local; do not weaken either regression. Minimal repair: add --verbose only to the shared ChromeDriver argv, and normalize a str chunk to UTF-8 bytes inside the drain boundary before feeding _ChromeDriverStartupDiagnostic, while rejecting other chunk types and continuing to discard raw content. Preserve the binary PIPE in production, no --log-path, no retry/no --no-sandbox, 3-trial denominators, and the closed sandbox_unavailable|unknown evidence vocabulary. Exact real-browser artifact 10068188981 still reports session-create 0/3 with startup_reason=unknown, so no sandbox-helper causality is established until this repository RED is fixed and the pinned browser is rerun.
seonghobae
left a comment
There was a problem hiding this comment.
Current-head follow-up on the credential-safe ChromeDriver diagnostic repair: the async drain and the session-failure classifier are not synchronized. _start_chromedriver launches a daemon thread, but _create_chromedriver_session checks diagnostic.startup_reason immediately when the W3C session not created response arrives. If ChromeDriver emits No usable sandbox just before/after that response and the drain thread has not consumed the relevant bytes yet, a genuine reviewed process diagnostic is still recorded as unknown. Adding --verbose and StringIO normalization alone can therefore make repository contracts GREEN while leaving the real-browser classifier timing-dependent.
Please keep the existing active source lease and add a RED that deterministically delays the diagnostic feed until the session error path has begun, then proves classification waits only for a bounded, explicit diagnostic handoff—not an arbitrary sleep and not process EOF. The minimum repair should expose a bounded synchronization primitive/state from the drain boundary (or an equivalent reviewed design), preserve continuous draining and raw-byte non-retention, and return unknown on timeout. Do not wait indefinitely for ChromeDriver to exit, do not retain logs, and do not change the browser trial denominator or sandbox policy. This is independent of whether the eventual root cause is the setuid helper/AppArmor; it is about making the closed classifier deterministic on the evidence it already owns.
|
Exact-head repair update for The source regressions from the earlier diagnostic review are repaired: the shared ChromeDriver launch retains Pinned Chrome for Testing remains a real hosted RED: run |
seonghobae
left a comment
There was a problem hiding this comment.
Current exact-head finding on 349fe818c49c8e0b92c890ee99c14e09d7d6f501: the new bounded diagnostic handoff is still timing-dependent. _ChromeDriverStartupDiagnostic.feed() sets _observed for every non-empty process chunk, and _create_chromedriver_session() calls wait_for_observation() only when the W3C error remains unknown. If ChromeDriver emits any benign verbose output before the reviewed No usable sandbox marker, _observed is already set; the wait returns immediately when session not created arrives, even if the drain thread has not yet consumed the later marker. The current regression only delays the marker from an otherwise silent diagnostic object, so it does not cover this ordering. Hosted artifact 10091005099 on this exact head still records browser-crash session creation 0/3 with startup_reason=unknown, which is consistent with the remaining race but does not by itself prove the sandbox marker was emitted.
Keep the continuous PIPE drain, --verbose, closed sandbox_unavailable|unknown vocabulary, raw-byte non-retention, no retry/no --no-sandbox, and all trial denominators. Add a deterministic RED where an unreviewed chunk is fed first, the W3C session error is raised, and the reviewed sandbox marker is deliberately delivered only after the current first-byte observation; the classifier must wait through a bounded explicit handoff for either reviewed-reason convergence or handoff expiry. A timeout must remain unknown; do not wait for process EOF or use arbitrary sleep as the production synchronization primitive. This is a repair finding under the existing active writer lane, not an approval or a request to rewrite history.
seonghobae
left a comment
There was a problem hiding this comment.
Exact-head follow-up on 884190f05bcb3ba19dcc11c950ac00c9e7abc17e: review 5150910010 is repaired at the source synchronization boundary. _ChromeDriverStartupDiagnostic no longer marks arbitrary non-empty verbose output as handoff completion; only recognition of the reviewed No usable sandbox marker sets _reviewed_reason, while wait_for_observation() retains the existing 250 ms bound and therefore leaves expiry as unknown. The focused regression now feeds ordinary ChromeDriver output before the W3C rejection and delivers the reviewed marker afterward, so the predecessor first-byte race is directly covered. Continuous PIPE drain, raw-byte non-retention, --verbose without --log-path, closed sandbox_unavailable|unknown, no retry/no --no-sandbox, and the existing trial denominators remain intact.
This is not approval or exact-head GREEN yet. CI 34322534771 and Manifest V3 Compatibility 34322534714 are currently in progress on this exact head. Do not transfer predecessor repository/browser evidence; use only their terminal exact-head results. If the real pinned-Chromium run still returns unknown, that remains fail-closed evidence rather than proof that no reviewed marker existed outside the bounded handoff window or that the sandbox helper is/non-is causal.
|
ACTIVE bounded PR-state lease on #148 at exact |
|
RELEASE bounded PR-state lease for #148 at exact |
Partial implementation of #28. Current exact head is
884190f05bcb3ba19dcc11c950ac00c9e7abc17e, base is #147 exact3dff28d9bf2dd27b72507e39979d51b8bf140fb4, and the PR remains open / Draft / mergeable.Buyer/security boundary
This slice owns credential-safe ChromeDriver/WebDriver startup diagnosis and browser-crash cleanup evidence. It does not own
.github/**, Chromium sandbox-helper installation, provider routing, or complete browser acceptance. All real-Chromium launch paths remain without--no-sandbox; a rejected session start is one failed trial, while driver/process/profile cleanup is cleanup evidence only.The process-output boundary is deliberately closed. ChromeDriver launches use one shared launch/session path with continuously drained verbose output, no diagnostic log file, and no raw process output retained. Only reviewed startup reason
sandbox_unavailableor fail-closedunknowncan enter evidence. Paths, arbitrary diagnostics, page-controlled data and secret-shaped values are not serialized.TDD / causal repair lineage
Test-first
5a86b28bb84643c3296c937a0480d70c9229ed6crequired a bounded_ChromeDriverStartupDiagnostic, chunk-boundary recognition of the reviewedNo usable sandboxmarker, continuous draining without payload retention/deadlock, shared PIPE-based ChromeDriver launch, and fail-closed unknown diagnostics. Exact CI34249189712produced the intended repository RED at Python contracts while production coverage remained independently GREEN.The following ordinary successors implemented and tightened that boundary. In particular, final
884190f05bcb3ba19dcc11c950ac00c9e7abc17efixes the asynchronous handoff race found in review5150969854: ordinary verbose output no longer completes the handoff early. Only recognition of the reviewed startup marker can signal early completion; otherwise the existing bounded handoff expires and the retained reason staysunknown. Process exit and arbitrary sleeps are not classification authority.The current source therefore distinguishes comparison/classification input from evidence payload and does not manufacture a more specific sandbox/AppArmor/helper cause than the bounded marker supplies.
Exact-head repository GREEN
Native CI
34322534771on exact884190f05bcb3ba19dcc11c950ac00c9e7abc17eis terminal success.102372220780passed Python repository contracts, canonical formatting, workspace check, full tests, strict Clippy, and rustdoc/API documentation.102372220458passed function/line/region/branch measurement, diagnostics publication, and exact coverage enforcement.This is repository-quality evidence for the exact source tree only. It is not browser GREEN.
Same-head real Chromium RED
Manifest V3 Compatibility
34322534714, job102372220261, checked out the same exact head, completed harden-runner setup, downloaded the exact pinned Chrome for Testing pair, then failed atExecute real MV3 compatibility fixture; bounded evidence upload succeeded.Artifact
10092466397, digestsha256:6e1b6cd6a11ef82633b40e035e5ef9aefec064f1514ec78f259e2513351fc7f4, records Chrome/ChromeDriver150.0.7871.129. The canonical #212 owner receipt records browser-crash trials 0/3 atsession_create / session_not_created / startup_reason=unknown; ordinary Agent Task, forced-close, and MV3 lanes likewise fail before a usable session. Profile cleanup does not convert those failures into success.This evidence does not prove that the cause is the archive sandbox helper, AppArmor, harden-runner, or another runner restriction.
unknownremainsunknownuntil canonical owner evidence proves otherwise.Canonical workflow / browser acceptance owners
Issue #212 owns current-generation MV3/browser workflow adoption. PR #43 is feasibility evidence that the pinned archive
chrome_sandboxcan work when root-owned, mode4755, and bound throughCHROME_DEVEL_SANDBOX; that stale leaf workflow is not canonical authority. Central reusable workflow work remains outside this product/process-diagnostic branch.#299 remains the pinned-Chromium presentation consumer and #292 the buyer acceptance owner. This #148 GREEN may be consumed only after normal non-destructive integration; it does not transfer browser acceptance. A later browser GREEN still requires the exact pinned-head causal sequence through successful session creation, browser-observed state/action/post-condition, and owned teardown/cleanup.
Remaining gates
Keep Draft. Normal promotion still requires canonical workflow-owner integration where applicable, a fresh exact consumer browser run that actually reaches the claimed behavior, current central security/review/ruleset satisfaction, parent-stack integration in dependency order, and no unresolved #28 browser/process/profile teardown gap.
No workflow/ruleset/secret mutation, Ready transition, self-approval, merge, tag, release, publication, force update, destructive restack,
--no-sandbox, trial reduction, or sandbox-policy weakening is part of this PR-state reconciliation.