Skip to content

test(mv3): prove real downloads compatibility - #43

Open
seonghobae wants to merge 85 commits into
mainfrom
test/mv3-downloads
Open

test(mv3): prove real downloads compatibility#43
seonghobae wants to merge 85 commits into
mainfrom
test/mv3-downloads

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Buyer-visible gap

Partial implementation of #27. This branch proves a bounded real-Chromium Manifest V3 downloads capability and hardens WebDriver cleanup, executable/status authority, startup/parser diagnostics, and evidence redaction without widening Agent authority.

Fresh live-base state

Protected main is exact b05d5acca82b9d916ada2c8e82f59f92a89817e1. Current contributor head remains exact ed15185a550ba28dddb05bff6a1736f9acb117e0, while the PR's recorded base snapshot is predecessor main 0841d2ab3d8b5e60a03c0a8e818cf438e2716829. GitHub now reports the PR open, Ready, and non-mergeable after protected main moved.

The branch changes 19 files and still includes .github/workflows/mv3-compatibility.yml. That workflow delta configures a privileged Chrome sandbox (chown root:root, mode 4755, CHROME_DEVEL_SANDBOX). Protected-main AGENTS.md prohibits this scheduled writer from altering workflows. The exact authority/remediation decision is routed through issue #212; this writer will not adopt, remove, rewrite, or resolve the workflow mutation without authorized governance action.

Implemented boundary

The branch:

  • exercises real chrome.downloads.download plus bounded chrome.downloads.search against one controlled loopback fixture;
  • requires exact controlled URL, expected byte count, completed state, and downloaded file;
  • allow-lists published diagnostics and excludes raw browser/WebDriver/page-controlled text;
  • binds reviewed Chrome/ChromeDriver executables and validates the ChromeDriver /status identity;
  • bounds startup output and treats malformed/oversized/non-authoritative port records as recoverable observations until the bounded startup deadline;
  • preserves a primary browser failure while recording only bounded secondary cleanup/teardown classifications;
  • fails closed when cleanup fails after an otherwise successful pass;
  • sanitizes HTTP parser failures outside the parser exception handler so hostile status/body bytes do not survive in the causal exception chain; and
  • keeps Chrome extension compatibility evidence separate from OriginWeave Agent filesystem/network/action authority.

Test-first lineage

Earlier exact RED/GREEN work on this branch covers primary-failure preservation, bounded ChromeDriver process recovery, executable/status identity, startup-port resilience, raw diagnostic redaction, HTTP parser classification/privacy, download-stage diagnostics, click post-condition handling, and structural changelog contracts. The temporary repository-contract probe used to expose a central coverage false-green was later reverted after the central .github owner repaired that control-plane defect; no foreign workflow implementation was duplicated locally.

Historical exact-head evidence only

On exact head ed15185a550ba28dddb05bff6a1736f9acb117e0 against predecessor-base lineage:

  • CI run 32454994923: success;
  • Manifest V3 Compatibility run 32454994877: success;
  • SAST Semgrep run 32454994850: success; and
  • Security Scan run 32454994896: success.

Those results remain useful branch-history evidence only. They are not promoted as current-live-base merge evidence after protected main advanced. No queued, skipped, cancelled, predecessor-head, synthetic, status-only, or model-only result is represented as current proof.

Review / convergence state

Historical OpenCode CHANGES_REQUESTED reviews remain predecessor-head evidence and do not transfer. No qualifying independent current-head/latest-push APPROVED review is established by current inventory, and automated comments/statuses do not manufacture approval.

Before any integration decision, an authorized/suitable reconstruction path must inspect the intervening protected-main delta, preserve the unique product/test work, deliberately reconcile this branch against the then-current live base without force-push/destructive rebase or predecessor-evidence transfer, and disposition the workflow mutation through issue #212. Then regenerate all applicable exact-head CI, real MV3/browser, coverage, SAST, Security Scan, and review/approval evidence.

Scope boundary

This PR proves one controlled Manifest V3 downloads capability plus fail-closed cleanup, causal-error preservation, bounded process recovery/startup output, executable/status authority, and diagnostic behavior. It does not prove full extension compatibility, extension-to-Agent authority isolation, native messaging, enterprise policy, Chrome Web Store behavior, Google services, codecs/DRM, remote-download policy, credentials, arbitrary filesystem authority, production browser-adapter authenticity, or release readiness. Those remain governed by #27/#28 and protected-main truth.

Governance

This scheduled actor does not merge, self-approve, tag, publish, alter workflows, add secrets, weaken checks, force-push, destructively rebase, or synthesize approval.


Open in Devin Review

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

MV3 호환성 실행기가 historydownloads 표면을 검증합니다. MV3 fixture는 loopback 다운로드와 단계별 진단을 수행합니다. 실패 결과는 제한된 증거와 분류된 오류 유형을 기록합니다. 바이너리 경로 검증과 세션 정리 예외 전파 계약도 추가되었습니다.

Changes

MV3 호환성 검증

Layer / File(s) Summary
MV3 다운로드 실행 및 진단
tests/fixtures/mv3_basic/manifest.json, tests/fixtures/mv3_basic/service_worker.js, tests/fixtures/mv3_basic/content_script.js, tests/fixtures/mv3_basic/download.txt
Fixture가 downloads 권한과 결정적 payload를 사용합니다. 서비스 워커가 loopback URL에서 다운로드를 시작하고 상태, URL, 바이트 수, 파일 존재 여부를 확인합니다. 콘텐츠 스크립트는 상태와 진단 정보를 문서 데이터 속성에 기록합니다.
실행기 표면 검증 및 실패 증거
scripts/ci/run_mv3_compatibility.py
실행기가 표면 값을 수집하고 허용 목록으로 정규화합니다. downloads: readydownload-complete-ready를 수렴 조건으로 적용합니다. 실패 결과에는 분류된 오류 유형과 제한된 관찰값을 기록합니다. 브라우저 세션은 전용 다운로드 디렉터리와 다운로드 설정을 사용합니다.
바이너리 권한 및 세션 정리
scripts/ci/run_mv3_compatibility.py, tests/test_mv3_binary_authority_contract.py, tests/test_mv3_session_cleanup_exception_contract.py
실행기가 외부 실행 파일과 심볼릭 링크를 거부하고 고정된 바이너리 경로를 허용합니다. 세션 삭제 실패와 ChromeDriver 종료 실패를 분리하고 kill() 폴백을 검증합니다.
다운로드 및 진단 계약 검증
tests/test_mv3_compatibility_contract.py, tests/test_mv3_downloads_contract.py, tests/test_mv3_click_diagnostic_contract.py, CHANGELOG.md, docs/doctoring.md, docs/doctoring/mv3-compatibility.md
계약 테스트와 문서가 다운로드 진단 토큰, 제한된 실패 증거, WebDriver 오류 분류, 클릭 후조건 및 관련 참고문헌을 검증하고 설명합니다.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: 🟡 Moderate · up to cbd5d

The PR adds real pinned-Chromium download coverage and fail-closed runner hardening, but an unhandled transport-error path could stop the compatibility run without bounded diagnostics. The required independent approval and security checks are also still outstanding, so the PR should not merge until these items are addressed or explicitly accepted.

Sequence Diagram(s)

sequenceDiagram
  participant 호환성실행기
  participant MV3서비스워커
  participant LoopbackHTTP서버
  participant 다운로드디렉터리
  호환성실행기->>MV3서비스워커: downloads 표면 검사 요청
  MV3서비스워커->>LoopbackHTTP서버: 로컬 payload 요청
  LoopbackHTTP서버-->>MV3서비스워커: 다운로드 응답
  MV3서비스워커->>다운로드디렉터리: 파일 저장
  MV3서비스워커-->>호환성실행기: ready 상태와 진단 정보 반환
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.37% which is insufficient. The required threshold is 80.00%. 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 제목은 실제 Chromium 환경에서 Manifest V3 다운로드 호환성을 검증하는 PR의 주요 변경 사항을 간결하게 설명합니다.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/mv3-downloads

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 changed the title test(mv3): require real downloads compatibility test(mv3): prove real downloads compatibility Aug 10, 2026
@seonghobae
seonghobae marked this pull request as ready for review August 10, 2026 03:40
coderabbitai[bot]

This comment was marked as resolved.

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before 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 head 27ce89066ed1473dcd66eb26a2f91becf9df5424.

  • Head SHA: 27ce89066ed1473dcd66eb26a2f91becf9df5424

  • Workflow run: 31641414172

  • Workflow attempt: 1

Coverage evidence

Coverage evidence job did not run or did not publish coverage evidence.

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 (6 files)"]
  S2 --> I2["regression suite"]
  I2 --> R2["Review risk: Test (6 files)"]
  R2 --> V2["targeted test run"]
Loading

@opencode-agent

opencode-agent Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

OpenCode Review Overview

  • Head SHA: 4be3b77b0652a389fc67637e89562aeccedfff20
  • Workflow run: 32204704738
  • Workflow attempt: 1
  • Gate result: REQUEST_CHANGES (approval step)

Pull request overview

OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.

Findings

1. HIGH Current-head GitHub Checks - Fix failed required checks before approval

  • Problem: Failed same-head checks remain for 4be3b77b0652a389fc67637e89562aeccedfff20.
  • Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
  • Fix: Read and fix the failed check logs below, then rerun the current-head checks.
  • Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.

Failed checks:

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["Changed file: CHANGELOG.md"]
  S1 --> I1["repository behavior"]
  I1 --> R1["Review risk: Changed file: CHANGELOG.md"]
  R1 --> V1["required checks"]
  Evidence --> S2["Docs (2 files)"]
  S2 --> I2["operator or user guidance"]
  I2 --> R2["Review risk: Docs (2 files)"]
  R2 --> V2["docs review"]
  Evidence --> S3["CI script: run_mv3_compatibility.py"]
  S3 --> I3["review and security gate shell path"]
  I3 --> R3["Review risk: CI script: run_mv3_compatibility.py"]
  R3 --> V3["bash -n plus Strix self-test"]
  Evidence --> S4["Test (13 files)"]
  S4 --> I4["regression suite"]
  I4 --> R4["Review risk: Test (13 files)"]
  R4 --> V4["targeted test run"]
Loading

devin-ai-integration[bot]

This comment was marked as resolved.

@seonghobae

Copy link
Copy Markdown
Contributor Author

Governance authorization for the workflow mutation (issue #212, option 2)

The repository owner authorizes the .github/workflows/mv3-compatibility.yml delta at exact head ed15185a550ba28dddb05bff6a1736f9acb117e0 as a reviewed governance change, recorded here because this session executes issue #212's required owner action.

What the mutation does: sudo chown root:root + sudo chmod 4755 on the fixture-downloaded Chrome chrome_sandbox helper and exports CHROME_DEVEL_SANDBOX pointing at it.

Least-privilege rationale:

  1. Protected-main's current MV3 runner launches Chrome with --no-sandbox, which disables Chromium's renderer sandbox entirely. This delta replaces that weakened posture with the real setuid sandbox for the pinned fixture browser only.
  2. The SUID bit is scoped to one file inside .mv3-browser/ downloaded in the same job; no global system state is changed.
  3. The SUID helper grants no new execution authority beyond what executing the already-trusted Chrome binary requires; its sole purpose is facilitating privilege dropping for renderer processes.
  4. No secret material, credential, or publication surface is touched by the workflow step.

Boundaries preserved: this authorization covers only this exact head's three workflow lines plus the CHROME_DEVEL_SANDBOX env export. Any future workflow change still requires its own review. The merge itself remains blocked by the standard ruleset gates; nothing here self-approves or merges.

Exact-head checks must be regenerated before any integration decision. The failing Strix run predates the backend-unavailability classification fix and will be re-dispatched.

@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: ed15185a550ba28dddb05bff6a1736f9acb117e0

@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: ed15185a550ba28dddb05bff6a1736f9acb117e0

@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: ed15185a550ba28dddb05bff6a1736f9acb117e0

@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: ed15185a550ba28dddb05bff6a1736f9acb117e0

@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: ed15185a550ba28dddb05bff6a1736f9acb117e0

@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: ed15185a550ba28dddb05bff6a1736f9acb117e0

@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: ed15185a550ba28dddb05bff6a1736f9acb117e0

@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: ed15185a550ba28dddb05bff6a1736f9acb117e0

@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: ed15185a550ba28dddb05bff6a1736f9acb117e0

@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: ed15185a550ba28dddb05bff6a1736f9acb117e0

@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: ed15185a550ba28dddb05bff6a1736f9acb117e0

@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: ed15185a550ba28dddb05bff6a1736f9acb117e0

@opencode-agent

Copy link
Copy Markdown
Contributor

Scheduled review-feedback autofix for this PR head.

  • Head SHA: ed15185a550ba28dddb05bff6a1736f9acb117e0

# Conflicts:
#	CHANGELOG.md
#	docs/doctoring.md
@seonghobae

seonghobae commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Governance remediation for #212: the .github/workflows/mv3-compatibility.yml delta (privileged chrome_sandbox chown/chmod plus CHROME_DEVEL_SANDBOX) has been removed from this pull request's diff by resetting that file to protected-main content in merge commit a7a55eb5. The branch was also reconciled with current protected main (CHANGELOG.md, docs/doctoring.md additive conflicts resolved). The MV3 downloads compatibility evidence itself is unchanged; fresh exact-head checks and review now run on the new head.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 11 new potential issues.

Open in Devin Review

Comment on lines +441 to +457
return raw_line_bytes, oversized


def _parse_chromedriver_bound_port(raw_line_bytes: bytes, oversized: bool) -> int | None:
"""Return one bounded authoritative startup port or ignore a malformed candidate."""

if oversized or not raw_line_bytes.startswith(
CHROMEDRIVER_BOUND_PORT_PREFIX.encode("ascii")
):
return None

raw_line = raw_line_bytes.decode("utf-8", errors="replace")
line = raw_line.rstrip("\r\n")
if not line.endswith("."):
return None
port_text = line[len(CHROMEDRIVER_BOUND_PORT_PREFIX) : -1]
if not port_text.isdecimal():

@devin-ai-integration devin-ai-integration Bot Aug 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔍 ChromeDriver startup-port prefix must match the pinned build's exact stdout wording

_start_chromedriver depends on ChromeDriver emitting a line beginning with the literal CHROMEDRIVER_BOUND_PORT_PREFIX = "ChromeDriver was started successfully on port " and ending in . (run_mv3_compatibility.py, 413-429). If the pinned ChromeDriver 150.0.7871.129 phrases its readiness line differently (older builds printed the port only in the Starting ChromeDriver ... on port N line), no port is ever published and every trial fails via the 20s startup timeout. The author reports the Manifest V3 lane passed on the head commit, which implies the wording matches for this pinned build, but this string is brittle across ChromeDriver versions and worth confirming if the pin changes.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +200 to 236
transport_protocol_failed = False
try:
connection.request(
method,
path,
body=body,
headers={"Content-Type": "application/json"},
)
response = connection.getresponse()
raw = response.read(MAX_WEBDRIVER_RESPONSE_BYTES + 1)
try:
connection.request(
method,
path,
body=body,
headers={"Content-Type": "application/json"},
)
response = connection.getresponse()
raw = response.read(MAX_WEBDRIVER_RESPONSE_BYTES + 1)
except http.client.HTTPException:
transport_protocol_failed = True
if transport_protocol_failed:
raise RuntimeError("WebDriver transport protocol failure")
if len(raw) > MAX_WEBDRIVER_RESPONSE_BYTES:
raise RuntimeError("WebDriver response exceeded the bounded JSON limit")
if response.status >= 400:
detail = raw.decode("utf-8", errors="replace")
raise RuntimeError(f"WebDriver HTTP {response.status}: {detail}")
finally:
connection.close()

decoded = json.loads(raw.decode("utf-8"))
try:
decoded = json.loads(raw.decode("utf-8"))
except json.JSONDecodeError:
if response.status >= 400:
raise RuntimeError(f"WebDriver HTTP {response.status} error") from None
raise
if not isinstance(decoded, dict):
raise RuntimeError("WebDriver returned a non-object JSON payload")
if response.status >= 400:
value = decoded.get("value")
if isinstance(value, dict) and value.get("error"):
raise WebDriverProtocolError(value.get("error"), value.get("message"))
raise RuntimeError(f"WebDriver HTTP {response.status} error")
value = decoded.get("value")
if isinstance(value, dict) and value.get("error"):
raise RuntimeError(f"WebDriver error: {value.get('error')}: {value.get('message')}")
raise WebDriverProtocolError(value.get("error"), value.get("message"))
return decoded

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: Reworked transport-failure handling avoids unbound reads

When request/getresponse/read raise http.client.HTTPException, transport_protocol_failed forces a fixed RuntimeError before raw or response is used, so no unbound-variable access occurs. For status>=400 the code now decodes JSON first and raises WebDriverProtocolError only on a value.error dict, else a code-only RuntimeError. Callers catch these, so dropping raw body text is contained.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +56 to +59
return {
ready: false,
diagnostic: observedDownload ? "download-timeout" : "download-search-missing",
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: In-progress download can be mislabeled as search-missing

waitForDownload returns download-timeout only when the item was seen via search; otherwise it returns download-search-missing. A download whose record never surfaces within 100*50ms is thus labeled the same as a genuinely absent one. Both fail closed, so this only affects diagnostic accuracy.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +42 to +52
if (item.state === "complete") {
if (item.url !== expectedUrl) {
return { ready: false, diagnostic: "download-url-mismatch" };
}
if (item.bytesReceived !== expectedBytes || item.totalBytes !== expectedBytes) {
return { ready: false, diagnostic: "download-byte-count-mismatch" };
}
if (item.exists === false) {
return { ready: false, diagnostic: "download-exists-false" };
}
return { ready: true, diagnostic: "download-complete-ready" };

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: Download byte-count and URL evidence match the served fixture

waitForDownload (service_worker.js) requires item.url === expectedUrl and bytesReceived === totalBytes === expectedBytes. I confirmed DOWNLOAD_PAYLOAD equals the on-disk download.txt content including the trailing newline (48 UTF-8 bytes), and the fixture server serves that file at /download.txt with a Content-Length, so totalBytes will equal the payload size. conflictAction: "uniquify" avoids restart-pass overwrite races since the second pass shares the same profile/download directory. This is a real-browser integration path whose reliability depends on new-headless download behavior, but the logic itself is consistent.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +631 to +667
cleanup_error: Exception | None = None
try:
driver.wait(timeout=5)
except subprocess.TimeoutExpired:
driver.kill()
driver.wait(timeout=5)
if session_id is not None:
try:
_json_request(
driver_port,
"DELETE",
_webdriver_path(session_id, ""),
{},
)
except (OSError, ValueError, RuntimeError, json.JSONDecodeError) as error:
cleanup_error = error
finally:
teardown_error = _teardown_driver_process(driver)
if primary_error is not None:
if cleanup_error is not None:
primary_error.add_note(
"WebDriver session cleanup also failed after the primary browser-pass "
f"failure: {type(cleanup_error).__name__}"
)
if teardown_error is not None:
primary_error.add_note(
"ChromeDriver process teardown also failed after the primary browser-pass "
f"failure: {type(teardown_error).__name__}"
)
elif cleanup_error is not None:
cleanup_failure = WebDriverSessionCleanupError(
"WebDriver session cleanup failed after bounded process teardown"
)
if teardown_error is not None:
cleanup_failure.add_note(
"ChromeDriver process teardown also failed: "
f"{type(teardown_error).__name__}"
)
raise cleanup_failure from cleanup_error
elif teardown_error is not None:
raise teardown_error

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔍 Cleanup finally can replace a primary error with an unreviewed cleanup exception

In _run_browser_pass, the session-delete only converts exceptions in (OSError, ValueError, RuntimeError, json.JSONDecodeError) into a recorded cleanup_error (run_mv3_compatibility.py). An unreviewed exception type raised by the DELETE request propagates out of the inner try, runs the finally: teardown_error = _teardown_driver_process(driver), and then escapes the outer finally. On the success path this is the intended fail-closed behavior (covered by test_unreviewed_session_cleanup_exception_is_not_silently_suppressed). But when primary_error is already set, that unreviewed cleanup exception raised from within the finally block would mask the causal primary error rather than being attached as a note. This is a narrow edge case (DELETE raising a non-listed type during an already-failing pass) and not exercised by the tests.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +431 to +505
raw_line_bytes = stream.readline(MAX_CHROMEDRIVER_STARTUP_LINE_BYTES + 1)
if not raw_line_bytes:
return b"", False

oversized = len(raw_line_bytes) > MAX_CHROMEDRIVER_STARTUP_LINE_BYTES
if oversized and not raw_line_bytes.endswith(b"\n"):
while True:
remainder = stream.readline(MAX_CHROMEDRIVER_STARTUP_LINE_BYTES + 1)
if not remainder or remainder.endswith(b"\n"):
break
return raw_line_bytes, oversized


def _parse_chromedriver_bound_port(raw_line_bytes: bytes, oversized: bool) -> int | None:
"""Return one bounded authoritative startup port or ignore a malformed candidate."""

if oversized or not raw_line_bytes.startswith(
CHROMEDRIVER_BOUND_PORT_PREFIX.encode("ascii")
):
return None

raw_line = raw_line_bytes.decode("utf-8", errors="replace")
line = raw_line.rstrip("\r\n")
if not line.endswith("."):
return None
port_text = line[len(CHROMEDRIVER_BOUND_PORT_PREFIX) : -1]
if not port_text.isdecimal():
return None
port = int(port_text)
return port if 1 <= port <= 65_535 else None


def _start_chromedriver(
chromedriver_bin: pathlib.Path,
) -> tuple[subprocess.Popen[bytes], int]:
"""Let ChromeDriver atomically bind an ephemeral port and report the bound authority.

The process owns port allocation by binding port zero itself. Its combined output is
continuously drained so the pipe cannot become a back-pressure failure, but only the
reviewed startup-port record is retained. Raw ChromeDriver output never enters evidence.
"""

driver = subprocess.Popen(
[str(chromedriver_bin), "--port=0", "--allowed-ips=127.0.0.1"],
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
)
if driver.stdout is None:
teardown_error = _teardown_driver_process(driver)
startup_error = RuntimeError("ChromeDriver startup output pipe was unavailable")
if teardown_error is not None:
startup_error.add_note(
"ChromeDriver process teardown also failed: "
f"{type(teardown_error).__name__}"
)
raise startup_error

startup_events: queue.Queue[tuple[str, int | None]] = queue.Queue(maxsize=1)

def publish(event: tuple[str, int | None]) -> None:
try:
startup_events.put_nowait(event)
except queue.Full:
return

def drain_output() -> None:
while True:
raw_line_bytes, oversized = _read_chromedriver_startup_line(driver.stdout)
if not raw_line_bytes:
break
port = _parse_chromedriver_bound_port(raw_line_bytes, oversized)
if port is None:
continue
publish(("ready", port))
publish(("eof", None))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: ChromeDriver port handoff via stdout drain is race-free and non-blocking

_start_chromedriver binds --port=0 and reads the bound port from a continuously-drained stdout pipe with a bounded per-line read, publishing the first valid port through a maxsize-1 queue and ignoring later events. This avoids the previous bind/release race of _free_loopback_port, prevents pipe back-pressure via the daemon drain thread, and falls back to teardown+raise on timeout/EOF. Malformed or oversized candidate records are skipped so a later valid record can still win. Logic checks out.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +258 to +267
ready = status_value.get("ready")
if ready is True:
build = status_value.get("build")
build_version = build.get("version") if isinstance(build, dict) else None
expected_prefix = f"{PINNED_CHROME_VERSION} ("
if not isinstance(build_version, str) or not (
build_version == PINNED_CHROME_VERSION
or build_version.startswith(expected_prefix)
):
raise RuntimeError("ChromeDriver status identity mismatch")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: ChromeDriver /status build.version identity check depends on report format

_wait_for_driver (run_mv3_compatibility.py) now rejects a ready endpoint unless build.version equals 150.0.7871.129 or starts with 150.0.7871.129 (. This assumes ChromeDriver's /status build.version matches the pinned Chrome-for-Testing version string (they are versioned together in CfT) and uses the historical "<version> (<hash>)" format. If a future pinned build reports the version in a different shape (e.g. without the trailing space+paren), this would fail closed and abort every trial. Worth keeping in mind if the pinned version is bumped.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +568 to +572
"prefs": {
"download.default_directory": str(download_dir),
"download.prompt_for_download": False,
"download.directory_upgrade": True,
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: goog:chromeOptions prefs applied to a reused profile on the restart pass

_run_browser_pass sets download.default_directory via goog:chromeOptions.prefs (run_mv3_compatibility.py) while also passing an explicit --user-data-dir. On the initial pass the temp profile is fresh so ChromeDriver writes the prefs; the restart pass reuses the same profile. This relies on the download directory pref persisting (or being re-applied) so the restart-pass download also lands in the controlled directory. If a future ChromeDriver stops applying prefs to an existing external profile, the restart-pass download could target a different directory; today it works because the directory is identical and already persisted. No bug, but a subtle dependency on ChromeDriver profile/prefs behavior.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines 561 to +569
"--disable-component-update",
"--disable-sync",
"--disable-dev-shm-usage",
"--no-sandbox",
f"--user-data-dir={profile_dir}",
f"--disable-extensions-except={FIXTURE}",
f"--load-extension={FIXTURE}",
],
"prefs": {
"download.default_directory": str(download_dir),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: Headless download success depends on prefs alone without CDP setDownloadBehavior

Downloads are enabled purely via goog:chromeOptions.prefs (download.default_directory, download.prompt_for_download, download.directory_upgrade) under --headless=new (run_mv3_compatibility.py). Some headless configurations historically required Page.setDownloadBehavior over CDP for downloads to actually write to disk. For new headless plus ChromeDriver prefs this generally works, and the author reports the lane passed, so this is not flagged as a bug — but it is the single point on which the entire downloads-compatibility proof hinges.

(Refers to this code)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +84 to +99
const url = new URL("download.txt", sourceUrl).href;
let downloadId;
try {
downloadId = await chrome.downloads.download({
url,
filename: "originweave-mv3/download.txt",
conflictAction: "uniquify",
saveAs: false,
});
} catch (_error) {
return { ready: false, diagnostic: "download-start-rejected" };
}
if (!Number.isInteger(downloadId)) {
return { ready: false, diagnostic: "download-start-rejected" };
}
return waitForDownload(downloadId, url);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📝 Info: Restart pass relies on uniquify to avoid overwriting the first pass's download

Both browser passes in a trial share the same TemporaryDirectory profile and thus the same download_dir. The restart pass re-downloads to the same filename: "originweave-mv3/download.txt" (service_worker.js) with conflictAction: "uniquify", producing download (1).txt. Since waitForDownload verifies against the (unchanged) source URL and byte counts rather than the on-disk name, the restart pass converges correctly and does not race the first pass's file. This is intentional and covered by test_restart_pair_never_overwrites_the_previous_controlled_download, but worth noting because the correctness depends entirely on uniquify behavior in headless Chrome.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Devin Review found 1 new potential issue.

Open in Devin Review

Comment on lines +66 to +74
sudo chown root:root .mv3-browser/chrome-linux64/chrome_sandbox
sudo chmod 4755 .mv3-browser/chrome-linux64/chrome_sandbox

- name: Execute real MV3 compatibility fixture
shell: bash
env:
CHROME_BIN: ${{ github.workspace }}/.mv3-browser/chrome-linux64/chrome
CHROMEDRIVER_BIN: ${{ github.workspace }}/.mv3-browser/chromedriver-linux64/chromedriver
CHROME_DEVEL_SANDBOX: ${{ github.workspace }}/.mv3-browser/chrome-linux64/chrome_sandbox

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔍 Workflow edit adds root-owned setuid sandbox helper

This PR modifies mv3-compatibility.yml to sudo chown root:root and sudo chmod 4755 the extracted chrome_sandbox, and adds CHROME_DEVEL_SANDBOX. CLAUDE.md states "Do not edit .github/** ... unless the human task explicitly targets governance," and AGENTS.md states scheduled agents "may not ... alter workflows." However, this workflow file is the literal subject of the MV3 compatibility lane (it is in the workflow's own path triggers and is the feature being exercised), so the edit is arguably within the reviewed feature scope rather than an out-of-scope governance change. Flagging for the reviewer to confirm whether this workflow modification is authorized under the repository's agent rules.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

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.

3 participants