mirror of
https://github.com/pewdiepie-archdaemon/odysseus.git
synced 2026-10-06 06:52:20 +02:00
fix(browser): derive batch navigation outcome from command rows
A batch whose open succeeded but whose later command failed was recorded as a failed navigation, so a following observation was wrongly labelled stale. Use the per-command rows; when the outcome cannot be determined, treat the page as unknown instead of claiming either result.
This commit is contained in:
@@ -50,7 +50,7 @@ TurnContract, generic process containment (Wave 3-S), effects/provenance
|
||||
| F4 | Missing `session_id` used agent-browser's shared `default` session | A sessionless call gets an ephemeral session that is closed and verified before the call returns |
|
||||
| F5 | Launch failure left the daemon alive | Launch-failure output triggers forced cleanup and a truthful error |
|
||||
| F6 | Concurrent actions on one session raced one daemon | Per-session `asyncio.Lock` serializes actions |
|
||||
| F7 | Observation after a failed navigation silently showed the old page | Sessions track navigation generation, page URL and failed navigation; such observations are prefixed with an explicit stale notice and flagged `stale_observation` |
|
||||
| F7 | Observation after a failed navigation silently showed the old page | Sessions track navigation generation, page URL and failed navigation; such observations are prefixed with an explicit stale notice and flagged `stale_observation`. A batch's navigation outcome comes from its per-command rows; when it cannot be determined the page is treated as unknown |
|
||||
| F8 | Recovery recursed through `execute` with a model-visible retry flag and no overall deadline | One deadline per call (action timeout + 75s); at most one retry, only for local read-only HTML open; model-supplied `_odysseus_browser_retry` is ignored |
|
||||
| F9 | `research_navigator` passed `timeout`, which the tool ignored | Passes `timeout_ms` |
|
||||
| F10 | No lifecycle evidence | Every result carries `browser_lifecycle` with stages, timings, ownership, state and cleanup receipt |
|
||||
@@ -61,7 +61,7 @@ TurnContract, generic process containment (Wave 3-S), effects/provenance
|
||||
|
||||
## Lifecycle model
|
||||
|
||||
Session states: `idle`, `ready`, `navigation_failed`, `reset`, `timed_out`,
|
||||
Session states: `idle`, `ready`, `navigation_failed`, `navigation_unknown`, `reset`, `timed_out`,
|
||||
`failed`, `launch_failed`, `bootstrap_failed`, `cancelled`, `closed`. Any state
|
||||
reached by forced cleanup discards the page URL so nothing earlier remains
|
||||
observable. Ownership is `retained` for a chat session (bounded by
|
||||
@@ -113,3 +113,8 @@ process-lifecycle primitives from Wave 3-S/5B.
|
||||
server requires its owner task in `builtin_mcp.py`.
|
||||
- The stale-observation notice marks, but does not block, an observation after
|
||||
a failed navigation.
|
||||
- Forced cleanup waits synchronously, at most one second, for killed processes
|
||||
to exit, so it can run from cancellation without awaiting.
|
||||
- The recovery deadline covers the action and its retry. Post-action
|
||||
observations (page errors, settled snapshot, screenshot) keep their own
|
||||
20 second bounds outside it.
|
||||
|
||||
@@ -2757,6 +2757,33 @@ class PrivateBrowserTool:
|
||||
header = "\n".join(part for part in (title, url) if part)
|
||||
return {"ok": True, "url": url, "text": f"{header}\n\n{text}".strip()}
|
||||
|
||||
@staticmethod
|
||||
def _batch_navigation_outcome(output: str, command_ok: bool) -> tuple[str, str]:
|
||||
"""Outcome of a batch's last navigation: ``ok``, ``failed`` or ``unknown``.
|
||||
|
||||
A later command failing does not undo a navigation that succeeded,
|
||||
so the per-command rows decide, not the batch exit status.
|
||||
"""
|
||||
|
||||
try:
|
||||
rows = json.loads(output)
|
||||
except (ValueError, TypeError):
|
||||
rows = None
|
||||
if isinstance(rows, list):
|
||||
for row in reversed(rows):
|
||||
command = row.get("command") if isinstance(row, dict) else None
|
||||
if not (
|
||||
isinstance(command, list)
|
||||
and command
|
||||
and str(command[0]).lower() in {"open", "goto", "navigate"}
|
||||
):
|
||||
continue
|
||||
if row.get("success") is True:
|
||||
result = row.get("result") if isinstance(row.get("result"), dict) else {}
|
||||
return "ok", str(result.get("url") or "")
|
||||
return "failed", ""
|
||||
return ("ok", "") if command_ok else ("unknown", "")
|
||||
|
||||
@staticmethod
|
||||
def _navigated_url(output: str) -> str:
|
||||
"""Final URL reported by ``open`` (after redirects), when present."""
|
||||
@@ -3028,12 +3055,20 @@ class PrivateBrowserTool:
|
||||
"untrusted_content": True,
|
||||
}
|
||||
if navigation_url:
|
||||
if command_ok:
|
||||
outcome, final_url = "ok" if command_ok else "failed", ""
|
||||
if action == "batch":
|
||||
outcome, final_url = self._batch_navigation_outcome(out, command_ok)
|
||||
if outcome == "ok":
|
||||
browser.navigated(
|
||||
(read_page or {}).get("url") or self._navigated_url(out) or navigation_url
|
||||
final_url
|
||||
or (read_page or {}).get("url")
|
||||
or self._navigated_url(out)
|
||||
or navigation_url
|
||||
)
|
||||
else:
|
||||
elif outcome == "failed":
|
||||
browser.navigation_failed(navigation_url)
|
||||
else:
|
||||
browser.navigation_unknown(navigation_url)
|
||||
if read_page is not None:
|
||||
if not command_ok:
|
||||
return {
|
||||
|
||||
@@ -300,6 +300,7 @@ class BrowserSession:
|
||||
navigation_generation: int = 0
|
||||
page_url: str = ""
|
||||
failed_navigation_url: str = ""
|
||||
navigation_outcome_unknown: bool = False
|
||||
_lock: asyncio.Lock | None = field(default=None, repr=False)
|
||||
_lock_loop: Any = field(default=None, repr=False)
|
||||
|
||||
@@ -321,22 +322,39 @@ class BrowserSession:
|
||||
self.navigation_generation += 1
|
||||
self.page_url = url
|
||||
self.failed_navigation_url = ""
|
||||
self.navigation_outcome_unknown = False
|
||||
self.state = "ready"
|
||||
|
||||
def navigation_failed(self, url: str) -> None:
|
||||
self.failed_navigation_url = url
|
||||
self.navigation_outcome_unknown = False
|
||||
self.state = "navigation_failed"
|
||||
|
||||
def navigation_unknown(self, url: str) -> None:
|
||||
"""A navigation was attempted but whether it happened is unknown."""
|
||||
|
||||
self.page_url = ""
|
||||
self.failed_navigation_url = url
|
||||
self.navigation_outcome_unknown = True
|
||||
self.state = "navigation_unknown"
|
||||
|
||||
def discarded(self, state: str) -> None:
|
||||
"""The browser and its page are gone; nothing earlier is observable."""
|
||||
|
||||
self.page_url = ""
|
||||
self.failed_navigation_url = ""
|
||||
self.navigation_outcome_unknown = False
|
||||
self.state = state
|
||||
|
||||
def stale_observation_note(self) -> str:
|
||||
if not self.failed_navigation_url:
|
||||
return ""
|
||||
if self.navigation_outcome_unknown:
|
||||
return (
|
||||
f"Browser lifecycle: the outcome of the most recent navigation to "
|
||||
f"{self.failed_navigation_url} is unknown. This observation may not "
|
||||
f"show {self.failed_navigation_url}."
|
||||
)
|
||||
shown = self.page_url or "an earlier page"
|
||||
return (
|
||||
f"Browser lifecycle: the most recent navigation to {self.failed_navigation_url} "
|
||||
|
||||
@@ -579,3 +579,41 @@ def test_selector_read_is_an_observation_not_a_navigation() -> None:
|
||||
assert PrivateBrowserTool._navigation_target(
|
||||
"batch", {"commands": [["open", "file:///a.html"], ["snapshot"], ["open", "file:///b.html"]]}
|
||||
) == "file:///b.html"
|
||||
|
||||
|
||||
def test_batch_navigation_outcome_comes_from_its_rows(browser_env) -> None:
|
||||
state, _, _, _ = browser_env
|
||||
responses = {}
|
||||
|
||||
async def _batch(command):
|
||||
if command[-2:] == ["batch", "--json"]:
|
||||
return responses["batch"]
|
||||
return 0, '- heading "x"'
|
||||
|
||||
state["behaviour"] = _batch
|
||||
ctx = {"session_id": "s-batch"}
|
||||
|
||||
# The open succeeded; a later click failing must not mark it failed.
|
||||
responses["batch"] = (1, json.dumps([
|
||||
{"command": ["open", "https://a.example/"], "success": True,
|
||||
"result": {"url": "https://a.example/landing"}},
|
||||
{"command": ["click", "@e9"], "success": False, "error": "no element"},
|
||||
]))
|
||||
result = _run({"action": "batch", "commands": [["open", "https://a.example/"], ["click", "@e9"]]}, ctx)
|
||||
assert result["browser_lifecycle"]["page_url"] == "https://a.example/landing"
|
||||
assert result["browser_lifecycle"]["state"] == "ready"
|
||||
assert "stale_observation" not in _run({"action": "snapshot"}, ctx)["browser_lifecycle"]
|
||||
|
||||
responses["batch"] = (1, json.dumps([
|
||||
{"command": ["open", "https://b.example/"], "success": False, "error": "net::ERR"},
|
||||
]))
|
||||
failed = _run({"action": "batch", "commands": [["open", "https://b.example/"]]}, ctx)
|
||||
assert failed["browser_lifecycle"]["state"] == "navigation_failed"
|
||||
note = _run({"action": "snapshot"}, ctx)["output"]
|
||||
assert "shows https://a.example/landing (navigation #1), not https://b.example/" in note
|
||||
|
||||
responses["batch"] = (1, "daemon connection lost")
|
||||
_run({"action": "batch", "commands": [["open", "https://c.example/"]]}, ctx)
|
||||
unknown = _run({"action": "snapshot"}, ctx)
|
||||
assert "outcome of the most recent navigation to https://c.example/ is unknown" in unknown["output"]
|
||||
assert unknown["browser_lifecycle"]["page_url"] == ""
|
||||
|
||||
@@ -90,7 +90,7 @@ The source tree reads **109** `ODYSSEUS_*` variables: 79 an operator may want to
|
||||
| `ODYSSEUS_BROWSER_MCP_REQUIRE_CACHE` | `''` | `src/builtin_mcp.py:90` | Truthy refuses to start the browser MCP server unless its npm package is already in the npx cache, instead of installing it at startup. |
|
||||
| `ODYSSEUS_BROWSER_NAMESPACE` | `'odysseus-ui'` | `src/agent_tools/web_tools.py:100` (+3 more) | Namespace for the detached agent-browser daemon's pid files, so two runtimes on one machine do not terminate each other's browsers. |
|
||||
| `ODYSSEUS_BROWSER_NO_SANDBOX` | `'1'` | `src/builtin_mcp.py:142` | Security-relevant. On by default, adding `--no-sandbox` because the Docker image cannot use the Chromium sandbox. Set 0, false or no to keep it. |
|
||||
| `ODYSSEUS_BROWSER_SCREENSHOT_DIR` | *unset* | `src/agent_tools/web_tools.py:3423` | Where private-browser screenshots are written. Falls back to the container path, then the system temp directory. |
|
||||
| `ODYSSEUS_BROWSER_SCREENSHOT_DIR` | *unset* | `src/agent_tools/web_tools.py:3458` | Where private-browser screenshots are written. Falls back to the container path, then the system temp directory. |
|
||||
|
||||
### Container and workspace mounts
|
||||
|
||||
|
||||
Reference in New Issue
Block a user