test(browser): execute controlled Agent Task on pinned Chrome - #70
test(browser): execute controlled Agent Task on pinned Chrome#70seonghobae wants to merge 17 commits into
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: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| agent_task_surfaces_complete = all( | ||
| trial.get("post_condition") is True | ||
| and trial.get("input_echo_verified") is True | ||
| and trial.get("url_unchanged") is True | ||
| and trial.get("extensions_disabled") is True | ||
| and trial.get("profile_cleaned") is True | ||
| for trial in agent_task_trials | ||
| if trial.get("passed") is True | ||
| ) |
There was a problem hiding this comment.
📝 Info: Agent Task surface gate is largely redundant
_agent_task_surfaces_complete requires five boolean fields true, but _run_agent_task_browser_pass raises before returning unless all are satisfied, so any passed trial already carries them true. The gate is effectively equivalent to the successful-trial count check that precedes it.
Was this helpful? React with 👍 or 👎 to provide feedback.
| _json_request( | ||
| driver_port, | ||
| "POST", | ||
| _element_command_path(session_id, input_element, "/value"), | ||
| {"text": AGENT_TASK_INPUT_VALUE, "value": list(AGENT_TASK_INPUT_VALUE)}, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Send Keys payload includes both W3C 'text' and legacy 'value' keys
The element /value request at run_mv3_compatibility.py sends {"text": AGENT_TASK_INPUT_VALUE, "value": list(AGENT_TASK_INPUT_VALUE)}. W3C WebDriver Element Send Keys uses text; the extra legacy value array is ignored by modern ChromeDriver in W3C mode and is harmless. The preceding /clear ensures the pre-populated input default (synthetic order 42) is removed before typing, so the subsequent text != AGENT_TASK_INPUT_VALUE check would fail closed if clear were ineffective — no false positive risk here.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| def _cleanup_browser_session_preserving_primary( | ||
| driver_port: int, | ||
| session_id: str, | ||
| primary_error: BaseException | None, | ||
| ) -> None: | ||
| """Fail closed on expected cleanup errors while retaining an earlier causal failure.""" | ||
|
|
||
| try: | ||
| _cleanup_browser_session(driver_port, session_id) | ||
| except (OSError, ValueError, RuntimeError, json.JSONDecodeError) as cleanup_error: | ||
| bounded_error = BrowserSessionCleanupError(cleanup_error) | ||
| if primary_error is None: | ||
| raise bounded_error from cleanup_error | ||
| raise bounded_error from primary_error |
There was a problem hiding this comment.
📝 Info: Cleanup wrapper only normalizes a fixed exception tuple
_cleanup_browser_session_preserving_primary catches only (OSError, ValueError, RuntimeError, json.JSONDecodeError). http.client.HTTPException subclasses (e.g. IncompleteRead) are none of these and would propagate uncaught, escaping the same narrow tuple caught in the main trial loop. Consistent with the existing pattern, so not a regression, but a malformed DELETE response would crash the runner rather than mark a failed trial.
Was this helpful? React with 👍 or 👎 to provide feedback.
| finally: | ||
| if session_id is not None: | ||
| with contextlib.suppress(Exception): | ||
| _json_request( | ||
| primary_error = sys.exc_info()[1] | ||
| try: | ||
| if session_id is not None: | ||
| _cleanup_browser_session_preserving_primary( | ||
| driver_port, | ||
| "DELETE", | ||
| _webdriver_path(session_id, ""), | ||
| {}, | ||
| session_id, | ||
| primary_error, | ||
| ) | ||
| driver.terminate() | ||
| try: | ||
| driver.wait(timeout=5) | ||
| except subprocess.TimeoutExpired: | ||
| driver.kill() | ||
| driver.wait(timeout=5) | ||
| finally: | ||
| driver.terminate() | ||
| try: | ||
| driver.wait(timeout=5) | ||
| except subprocess.TimeoutExpired: | ||
| driver.kill() | ||
| driver.wait(timeout=5) |
There was a problem hiding this comment.
📝 Info: Cleanup failures now fail the trial (behavior change)
Both browser-pass finalizers read sys.exc_info()[1] to chain cleanup errors onto the causal failure. This replaces the old contextlib.suppress(Exception), which silently discarded cleanup errors. A WebDriver session or profile cleanup failure now converts an otherwise passing trial into a failed one — fail-closed, and the tests exercise the chaining paths directly.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "duration_ms": round((time.monotonic() - started) * 1000), | ||
| } | ||
| finally: | ||
| primary_error = sys.exc_info()[1] |
There was a problem hiding this comment.
📝 Info: sys.exc_info() primary-error detection is context-fragile
The three finally blocks read primary_error from sys.exc_info()[1]. This is correct only because no caller invokes these functions from inside an active except block. A future caller doing so would surface a stale outer exception and mis-chain the cleanup error. Correct as currently called.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _cleanup_agent_task_browser_session(driver_port: int, session_id: str) -> None: | ||
| """Delete one Agent Task WebDriver session without suppressing cleanup failures.""" | ||
|
|
||
| _cleanup_browser_session(driver_port, session_id) |
There was a problem hiding this comment.
📝 Info: Cleanup wrapper tested but never used in production
_cleanup_agent_task_browser_session is only exercised by a test; the real Agent Task pass cleans up via _cleanup_browser_session_preserving_primary. The test guards a function that is not on the executed cleanup path.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Partial implementation of #28, stacked on PR #65.
Buyer-visible boundary
PR #65 provides the controlled Agent Task fixture. This lane proves pinned stock Chromium can execute the bounded clear/type/click/post-condition workflow without leaving the authorized fixture document, while preserving isolated-profile cleanup and bounded repeatability evidence.
Fresh stack state
Protected
mainisb05d5acca82b9d916ada2c8e82f59f92a89817e1. Live prerequisite #65 is93a85c1464ab73ed7386367dc78fbad9c4f2f107.The prior child was behind that moved prerequisite, so #70 was reconstructed non-destructively from the exact current #65 tree while preserving its six unique Agent Task/MV3 files. Current exact head is
4930ab764eb63c524c4129196bd2acb6188c1c02. Fresh comparison against #65 reports merge base exactly93a85c1464ab73ed7386367dc78fbad9c4f2f107, 14 ahead / 0 behind, with only the intended child files changed. No predecessor evidence transfers.TDD boundary
The runner uses pinned Chrome WebDriver clear/type/click operations against the controlled fixture, verifies submitted state and synthetic echo, requires the loaded URL to remain unchanged, emits bounded credential-free repeatability evidence, and proves temporary-profile cleanup. A prior security regression that reflected page-controlled
data-statetext into diagnostics was repaired to a constant failure message; no catch-all suppression or authority weakening was introduced.Exact-current evidence
On unchanged exact head
4930ab764eb63c524c4129196bd2acb6188c1c02:32935787043: success.98076633184: success, including repository contracts, formatting, workspace/all-target check, full tests, strict Clippy, and rustdoc under Rust 1.97.1.98076633012: success.9594817668, digestsha256:75aa3efbb13fa9f2cc4877eace0d1e564f08cd412002da8c9b74c1feb9bb8496.32935787063: success.98076633056: success.9594787798, digestsha256:e1fa350ff31fd076900c5d2a63bd2bdb75bfd6882be87d27eaabf21f2fde6bd1.150.0.7871.129, Chromium revisionr1639810.post_condition,input_echo_verified,url_unchanged,extensions_disabled, andprofile_cleanedtrue.No same-head SAST or Security Scan success is claimed because the reconstructed head currently exposes CI and Manifest V3 evidence only.
Truth / authority boundary
This is executable controlled-browser evidence, not the complete OriginWeave browser runtime. It does not prove full origin authorization, BiDi/CDP product-adapter semantics, semantic node-handle composition, policy dispatch, provenance, trusted process attribution, secret brokerage, persistence, or closure of #28. #65 remains active, so this child is dependency-bound. Protected-main
AGENTS.mdremains authoritative; this scheduled actor does not merge, self-approve, force-push, alter workflows/rulesets, add secrets, weaken checks, tag, release, or publish.