[codex] add Firefox RDP launcher and sync fallback#110
Open
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds Firefox remote-debug auto-launch and managed profiles; extends browser-session handling for CDP, Firefox RDP and BiDi; adds CLI session sync/check/start-debug commands; updates config TOML serialization and WebSocket handshake docs; and adds extensive tests and Makefile targets. Changes
Sequence Diagram(s)sequenceDiagram
actor CLI
participant Config
participant Launcher as Browser\nLauncher
participant Firefox
participant DebugServer as Debug\nServer
CLI->>Config: read FIREFOX_DEBUG_* / browser_debug_url
Config-->>CLI: config values
CLI->>Launcher: get_firefox_debug_launch_spec(debug_url)
Launcher->>Launcher: validate local ws:// URL & extract port
alt spec available & server unreachable
Launcher->>Launcher: ensure_managed_firefox_profile() -> write user.js/prefs.js
Launcher->>Firefox: start subprocess (--profile, --start-debugger-server ws:PORT, --new-window)
Firefox->>DebugServer: bind debugger server on PORT
loop retry window
Launcher->>DebugServer: attempt websocket connect
DebugServer-->>Launcher: initial message ({"from":"root"}) or timeout
end
Launcher-->>CLI: true (launched & ready)
else server already reachable or no-spec
Launcher-->>CLI: false (no launch)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| def _is_gengo_rss_feed(self) -> bool: | ||
| feed_url = str(self.config.get("Watcher", "feed_url") or "") | ||
| return "gengo.com" in feed_url.lower() |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for syncing WebSocket sessions from live browser instances, specifically adding support for Firefox via BiDi and RDP protocols. Key changes include a new browser_debug_launcher module, updated CLI commands for session management, and enhanced GengoWatcher logic to handle session snapshots and automated browser activity. The WebSocket handshake now includes browser-aligned headers, and the authentication payload has been simplified. Feedback focuses on improving the robustness of cookie value coercion and adding a timeout to the initial Firefox RDP handshake to prevent potential hangs.
Comment on lines
+249
to
+268
| def _coerce_cookie_value(value: Any) -> str: | ||
| if isinstance(value, str): | ||
| return value.strip() | ||
| if not isinstance(value, dict): | ||
| return str(value or "").strip() | ||
|
|
||
| value_type = str(value.get("type") or "") | ||
| raw_value = value.get("value", "") | ||
| if value_type == "string": | ||
| return str(raw_value or "").strip() | ||
| if value_type == "base64": | ||
| try: | ||
| return ( | ||
| base64.b64decode(str(raw_value or ""), validate=False) | ||
| .decode("utf-8", errors="replace") | ||
| .strip() | ||
| ) | ||
| except (ValueError, TypeError): | ||
| return "" | ||
| return str(raw_value or "").strip() |
Contributor
There was a problem hiding this comment.
The _coerce_cookie_value function uses value or "" which can lead to incorrect results for numeric values like 0 or boolean False, as they are falsy in Python and will be coerced to an empty string. While cookie values are typically strings, it is safer to explicitly check for None to preserve other falsy values that might be valid cookie content.
Suggested change
| def _coerce_cookie_value(value: Any) -> str: | |
| if isinstance(value, str): | |
| return value.strip() | |
| if not isinstance(value, dict): | |
| return str(value or "").strip() | |
| value_type = str(value.get("type") or "") | |
| raw_value = value.get("value", "") | |
| if value_type == "string": | |
| return str(raw_value or "").strip() | |
| if value_type == "base64": | |
| try: | |
| return ( | |
| base64.b64decode(str(raw_value or ""), validate=False) | |
| .decode("utf-8", errors="replace") | |
| .strip() | |
| ) | |
| except (ValueError, TypeError): | |
| return "" | |
| return str(raw_value or "").strip() | |
| def _coerce_cookie_value(value: Any) -> str: | |
| if isinstance(value, str): | |
| return value.strip() | |
| if not isinstance(value, dict): | |
| return str(value).strip() if value is not None else "" | |
| value_type = str(value.get("type") or "") | |
| raw_value = value.get("value", "") | |
| if value_type == "string": | |
| return str(raw_value).strip() if raw_value is not None else "" | |
| if value_type == "base64": | |
| try: | |
| return ( | |
| base64.b64decode(str(raw_value or ""), validate=False) | |
| .decode("utf-8", errors="replace") | |
| .strip() | |
| ) | |
| except (ValueError, TypeError): | |
| return "" | |
| return str(raw_value).strip() if raw_value is not None else "" |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/gengowatcher/config.py (2)
390-394:⚠️ Potential issue | 🟡 Minor
_backfill_from_legacy_config()is called twice — the second call is redundant work on every load.Line 391 backfills, then line 393 validates, then line 394 backfills again. The second invocation re-reads
config.ini, re-parses every key, and potentially re-writes the TOML file. Presumably a merge artifact.🔧 Proposed fix
# Backfill from legacy config before validation self._backfill_from_legacy_config() # Validate auto-accept configuration after backfill self._validate_auto_accept_config() - self._backfill_from_legacy_config()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gengowatcher/config.py` around lines 390 - 394, The second call to _backfill_from_legacy_config() is redundant and causes unnecessary re-parsing and potential rewrites; remove the duplicate invocation so the sequence is just self._backfill_from_legacy_config() followed by self._validate_auto_accept_config(), ensuring _backfill_from_legacy_config() runs exactly once before validation (confirm there are no other callers relying on the double call and run tests to verify no side effects).
593-604:⚠️ Potential issue | 🔴 CriticalCritical:
NameErrorwaiting to happen —jsonis never imported, and this tail is dead/duplicate code.Lines 593–596 re-check
booland(int, float)after they were already handled at lines 568–572, so they can never match. Line 597 then callsjson.dumps(value), butjsonisn't imported in this module (onlytomllib,copy,os,shutil,sys,threading,configparser,pathlib,typing). Anyvaluethat reaches line 597 (e.g., a tuple, set, or custom object) will raiseNameError: name 'json' is not definedinstead of serialising. Thetry/exceptblock on lines 599–604 is also unreachable because line 597 always returns. Ruff (F821) already flagged this.🔧 Proposed fix
if value is None: return '""' - if isinstance(value, bool): - return "true" if value else "false" - if isinstance(value, (int, float)): - return str(value) - return json.dumps(value) - - try: - return json.dumps(value) - except (TypeError, ValueError): - pass - return str(value)Also add
import jsonat the top if you genuinely needjson.dumpsas a fallback — but given the earlier branches cover all TOML-serialisable types,str(value)is likely fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gengowatcher/config.py` around lines 593 - 604, The tail duplicate block in src/gengowatcher/config.py re-checks bool/int/float and then calls json.dumps without importing json, causing a NameError; remove the unreachable duplicate branch (the second if isinstance(value, bool)/(int, float) and the immediate return json.dumps(value) plus the unreachable try/except) and either (A) simply return str(value) as the fallback for non-TOML-serialisable values, or (B) if you want JSON fallback, add import json at module top and replace the removed fallback with a single guarded json.dumps(value) inside a try/except (catching TypeError/ValueError) that falls back to str(value); update the function containing these branches accordingly.src/gengowatcher/main.py (1)
59-213:⚠️ Potential issue | 🟠 MajorDelete lines 60–213: this is dead code that will crash with NameError
Line 59 calls
run_application(args, console), which may return normally. The code that follows references undefined symbols—CategoryFilter,log,ui_handler,RotatingFileHandler,logging,threading,AppState,GengoWatcher,StatsManager,GengoWatcherApp, andtimeare not imported inmain.py. This block duplicates logic already handled correctly inruntime.pyand will immediately raiseNameErrorif executed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gengowatcher/main.py` around lines 59 - 213, Remove the dead/duplicated initialization and runtime block that follows run_application(args, console) (the code referencing CategoryFilter, log, ui_handler, RotatingFileHandler, logging, threading, AppState, GengoWatcher, StatsManager, GengoWatcherApp, time, etc.); delete that entire section (the lines duplicating runtime.py logic) so main.py does not reference undefined symbols or duplicate startup logic, and ensure main.py relies on the existing runtime.py behavior instead of reintroducing these classes/handlers.src/gengowatcher/watcher.py (1)
369-379:⚠️ Potential issue | 🟡 MinorMark live-but-quiet sockets as stale.
quiet_age_secis reported, and alerts special-casedetail.startswith("quiet "), butget_health_snapshotnever emits that detail. A socket can stay “healthy” forever on pongs alone while receiving no application messages.Proposed fix
elif self.websocket_status == "Live": - if last_pong_age is None: + quiet_stale_after = self._get_session_quiet_stale_seconds() + if quiet_age is not None and quiet_age >= quiet_stale_after: + ws_state = "stale" + ws_detail = f"quiet {int(quiet_age)}s" + elif last_pong_age is None: ws_state = "stale" ws_detail = "no pong" elif last_pong_age <= 40:Also applies to: 598-610
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gengowatcher/watcher.py` around lines 369 - 379, The health snapshot logic marks sockets "healthy" solely based on last_pong_age but never emits the "quiet Xs" detail; update the branches that set ws_state/ws_detail (the block that checks self.websocket_status == "Live" and the similar later block around the other Live/Offline handling) to also inspect quiet_age_sec: if quiet_age_sec is not None and exceeds your quiet threshold mark ws_state = "stale" and set ws_detail = f"quiet {int(quiet_age_sec)}s", otherwise preserve the existing pong-based logic (use last_pong_age <= 40 => "healthy" / "ok"). Ensure you update both occurrences that set ws_state and ws_detail so alert code that matches detail.startswith("quiet ") will receive the quiet detail.tests/test_browser_session.py (1)
924-928:⚠️ Potential issue | 🟡 MinorPatch the snapshot loader, not the legacy token helper.
These tests now bypass the primary CLI sync path. Patch
_load_browser_session_snapshotorfetch_browser_session_snapshot_syncso the tests cover the snapshot fields that the CLI writes.Proposed test direction
- with patch( - "gengowatcher.main.fetch_browser_session_token_sync", - return_value="fresh-token", - ): + with patch( + "gengowatcher.cli._load_browser_session_snapshot", + return_value=BrowserSessionSnapshot( + session_token="fresh-token", + user_key="browser-user-key", + user_agent="Helium Browser", + accept_language="en-GB,en-US;q=0.9", + ), + ): handled = handle_cli_config_commands(args, config, console=MagicMock())You’ll also need to import
BrowserSessionSnapshotfromgengowatcher.browser_session.Also applies to: 958-962
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_browser_session.py` around lines 924 - 928, The test currently patches fetch_browser_session_token_sync, bypassing the CLI snapshot path; instead patch the snapshot loader used by the CLI by mocking gengowatcher.browser_session._load_browser_session_snapshot or gengowatcher.main.fetch_browser_session_snapshot_sync to return a BrowserSessionSnapshot instance with the expected fields (import BrowserSessionSnapshot from gengowatcher.browser_session in the test), so handle_cli_config_commands exercises the snapshot write/read logic; apply the same change to the other occurrence mentioned (the block around the second patch).
♻️ Duplicate comments (1)
src/gengowatcher/watcher.py (1)
796-798:⚠️ Potential issue | 🟠 MajorParse the RSS hostname instead of substring matching.
"gengo.com" in feed_url.lower()also matches URLs likehttps://evil.example/?next=gengo.com, which is why CodeQL is failing here. Check the parsed hostname.Proposed fix
+from urllib.parse import urlparse + @@ def _is_gengo_rss_feed(self) -> bool: feed_url = str(self.config.get("Watcher", "feed_url") or "") - return "gengo.com" in feed_url.lower() + hostname = (urlparse(feed_url).hostname or "").lower() + return hostname == "gengo.com" or hostname.endswith(".gengo.com")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gengowatcher/watcher.py` around lines 796 - 798, The current _is_gengo_rss_feed method does substring-matching which can be fooled by query params; update _is_gengo_rss_feed to parse feed_url (use urllib.parse.urlparse) and extract the hostname, normalize to lowercase and then check the host for an exact match or a safe suffix (e.g., host == "gengo.com" or host.endswith(".gengo.com")); handle empty/invalid URLs and exceptions by returning False. Ensure you reference feed_url from self.config.get(...) as before and keep the method signature _is_gengo_rss_feed().
🧹 Nitpick comments (3)
tests/test_cli.py (1)
27-39: Solid coverage — optionally extend to the suppressed aliases.The three primary flags are covered. Since
--sync-sessionand--check-sessionare still wired as hidden aliases (help=argparse.SUPPRESS) incli.py, a one-line assertion each would lock in that behaviour so a future cleanup doesn't silently drop the short forms.🔧 Optional addition
assert should_handle_lightweight_command(firefox_args) is True + + +def test_browser_session_short_aliases_still_map_to_lightweight_commands(): + parser = build_argument_parser() + + sync_alias = parser.parse_args(["--sync-session"]) + check_alias = parser.parse_args(["--check-session"]) + + assert sync_alias.sync_session_from_browser is True + assert check_alias.check_session_from_browser is True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 27 - 39, Add assertions to the test_browser_session_flags_map_to_lightweight_commands test to also parse and assert the suppressed alias flags map to the same attributes and lightweight handling: call parser.parse_args with ["--sync-session"] and ["--check-session"], assert their sync_session_from_browser and check_session_from_browser attributes are True, and assert should_handle_lightweight_command(...) returns True for each; locate the parser via build_argument_parser and the test function test_browser_session_flags_map_to_lightweight_commands to add these one-line checks so the hidden aliases in cli.py remain covered.Makefile (1)
35-40: Optional: collapse four--setinvocations into one AppConfig load/save cycle.Each line reboots the Python interpreter, loads the full config, writes one key, then saves. Beyond being slow, a Ctrl-C between invocations leaves config half-updated. If
--setaccepts multiple triples (or a smallconfigure-firefox-debugsubcommand were added), this target becomes atomic. Not blocking — just a polish opportunity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 35 - 40, The firefox-debug Makefile target repeatedly invokes gengowatcher.main with four separate "--set" calls which restarts Python and reloads/saves AppConfig each time; change it to perform a single atomic configuration update by either (1) extending the CLI to accept multiple "--set <Section> <Key> <Value>" triples in one invocation and update the Makefile's firefox-debug recipe to pass all four triples in one gengowatcher.main call, or (2) add a new subcommand (e.g., configure-firefox-debug) in gengowatcher.main that loads AppConfig once, sets browser_debug_url, browser_debug_browser_path, browser_debug_profile_path and browser_debug_auto_launch, then saves; update the Makefile to call that single subcommand. Ensure the unique targets/functions affected are the Makefile target "firefox-debug" and the CLI entry point in gengowatcher.main that handles "--set" (or the new "configure-firefox-debug" handler) so the config write is done once atomically.tests/test_browser_debug_launcher.py (1)
14-29: Add negative coverage for the local-only debug boundary.This test proves a local
ws://127.0.0.1URL is accepted, but not that remote hosts,wss://, or/sessionendpoints are rejected. That boundary is important because the managed Firefox debug server is unauthenticated.Example coverage
def test_get_firefox_debug_launch_spec_requires_local_ws_url(): config = MagicMock() config.getboolean.return_value = True @@ assert spec.profile_path == Path("profiles/firefox-debug") assert spec.port == 6000 + + +def test_get_firefox_debug_launch_spec_rejects_non_local_or_non_rdp_urls(): + config = MagicMock() + config.getboolean.return_value = True + config.get.side_effect = lambda section, option, fallback=None: { + ("Paths", "browser_debug_browser_path"): "firefox-bin", + ("WebSocket", "browser_debug_profile_path"): "profiles/firefox-debug", + ("WebSocket", "browser_debug_start_url"): "https://gengo.com/t/jobs/", + }.get((section, option), fallback) + + assert get_firefox_debug_launch_spec(config, "ws://example.com:6000") is None + assert get_firefox_debug_launch_spec(config, "wss://127.0.0.1:6000") is None + assert get_firefox_debug_launch_spec(config, "ws://127.0.0.1:6000/session") is None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_browser_debug_launcher.py` around lines 14 - 29, Add negative tests alongside test_get_firefox_debug_launch_spec_requires_local_ws_url to assert get_firefox_debug_launch_spec rejects non-local or non-plain-ws endpoints: call get_firefox_debug_launch_spec with examples like "ws://192.168.1.2:6000", "wss://127.0.0.1:6000", and "ws://127.0.0.1:6000/session" (or any URL containing "/session") and assert the returned spec is None (or raises the same failure mode as the function uses). Reuse the same MagicMock config setup from the existing test and add separate assertions for each invalid URL case to ensure remote hosts, secure wss scheme, and session endpoints are rejected by get_firefox_debug_launch_spec.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gengowatcher/browser_debug_launcher.py`:
- Around line 59-65: The helper _looks_like_local_firefox_rdp_url currently
treats a None hostname as "127.0.0.1", which lets malformed URLs like
"ws:///session" be classified as local; update the function to require a real
hostname by checking parsed.hostname is not None and then testing
parsed.hostname in {"127.0.0.1", "localhost"} (instead of using (parsed.hostname
or "127.0.0.1")), preserving the existing scheme and path checks so only
well-formed local hosts return True.
- Around line 195-212: The websockets.connect call in
_probe_firefox_debug_server can hang during the opening handshake; update the
websockets.connect(...) call inside _probe_firefox_debug_server to include an
open_timeout=2 (e.g., websockets.connect(debug_url, max_size=1_000_000,
open_timeout=2)) so the handshake is bounded while keeping the existing
asyncio.wait_for recv timeout, then leave can_connect_to_firefox_debug_server
as-is to continue using asyncio.run(_probe_firefox_debug_server(debug_url)).
In `@src/gengowatcher/cli.py`:
- Around line 157-182: The broad except Exception around
fetch_browser_session_snapshot_sync should be narrowed to only catch the
expected browser-attach/timeout exception (e.g., BrowserAttachError or whatever
specific exception main_module raises) and re-raise any other exceptions; when
falling back to a token-only BrowserSessionSnapshot (created via
fetch_browser_session_token_sync) do not fabricate default
user_key/user_agent/accept_language—retrieve and reuse the existing values from
the runtime/config (e.g., read from config or a current-session accessor on
main_module) and only use FALLBACK_* constants as a last-resort fallback; keep
references to fetch_browser_session_snapshot_sync,
maybe_launch_managed_firefox_debug, fetch_browser_session_token_sync,
BrowserSessionSnapshot, and FALLBACK_BROWSER_* when implementing this change.
In `@src/gengowatcher/main.py`:
- Around line 13-16: Remove the unused re-exports from main.py: delete the
imports fetch_browser_session_snapshot_sync and fetch_browser_session_token_sync
from main_module and instead import them directly where needed in cli.py; update
cli.py to import fetch_browser_session_snapshot_sync and
fetch_browser_session_token_sync from the browser_session module at the point
they are used (rather than accessing them via main_module) so CLI concerns stay
in cli.py and main_module has no unnecessary coupling.
---
Outside diff comments:
In `@src/gengowatcher/config.py`:
- Around line 390-394: The second call to _backfill_from_legacy_config() is
redundant and causes unnecessary re-parsing and potential rewrites; remove the
duplicate invocation so the sequence is just self._backfill_from_legacy_config()
followed by self._validate_auto_accept_config(), ensuring
_backfill_from_legacy_config() runs exactly once before validation (confirm
there are no other callers relying on the double call and run tests to verify no
side effects).
- Around line 593-604: The tail duplicate block in src/gengowatcher/config.py
re-checks bool/int/float and then calls json.dumps without importing json,
causing a NameError; remove the unreachable duplicate branch (the second if
isinstance(value, bool)/(int, float) and the immediate return json.dumps(value)
plus the unreachable try/except) and either (A) simply return str(value) as the
fallback for non-TOML-serialisable values, or (B) if you want JSON fallback, add
import json at module top and replace the removed fallback with a single guarded
json.dumps(value) inside a try/except (catching TypeError/ValueError) that falls
back to str(value); update the function containing these branches accordingly.
In `@src/gengowatcher/main.py`:
- Around line 59-213: Remove the dead/duplicated initialization and runtime
block that follows run_application(args, console) (the code referencing
CategoryFilter, log, ui_handler, RotatingFileHandler, logging, threading,
AppState, GengoWatcher, StatsManager, GengoWatcherApp, time, etc.); delete that
entire section (the lines duplicating runtime.py logic) so main.py does not
reference undefined symbols or duplicate startup logic, and ensure main.py
relies on the existing runtime.py behavior instead of reintroducing these
classes/handlers.
In `@src/gengowatcher/watcher.py`:
- Around line 369-379: The health snapshot logic marks sockets "healthy" solely
based on last_pong_age but never emits the "quiet Xs" detail; update the
branches that set ws_state/ws_detail (the block that checks
self.websocket_status == "Live" and the similar later block around the other
Live/Offline handling) to also inspect quiet_age_sec: if quiet_age_sec is not
None and exceeds your quiet threshold mark ws_state = "stale" and set ws_detail
= f"quiet {int(quiet_age_sec)}s", otherwise preserve the existing pong-based
logic (use last_pong_age <= 40 => "healthy" / "ok"). Ensure you update both
occurrences that set ws_state and ws_detail so alert code that matches
detail.startswith("quiet ") will receive the quiet detail.
In `@tests/test_browser_session.py`:
- Around line 924-928: The test currently patches
fetch_browser_session_token_sync, bypassing the CLI snapshot path; instead patch
the snapshot loader used by the CLI by mocking
gengowatcher.browser_session._load_browser_session_snapshot or
gengowatcher.main.fetch_browser_session_snapshot_sync to return a
BrowserSessionSnapshot instance with the expected fields (import
BrowserSessionSnapshot from gengowatcher.browser_session in the test), so
handle_cli_config_commands exercises the snapshot write/read logic; apply the
same change to the other occurrence mentioned (the block around the second
patch).
---
Duplicate comments:
In `@src/gengowatcher/watcher.py`:
- Around line 796-798: The current _is_gengo_rss_feed method does
substring-matching which can be fooled by query params; update
_is_gengo_rss_feed to parse feed_url (use urllib.parse.urlparse) and extract the
hostname, normalize to lowercase and then check the host for an exact match or a
safe suffix (e.g., host == "gengo.com" or host.endswith(".gengo.com")); handle
empty/invalid URLs and exceptions by returning False. Ensure you reference
feed_url from self.config.get(...) as before and keep the method signature
_is_gengo_rss_feed().
---
Nitpick comments:
In `@Makefile`:
- Around line 35-40: The firefox-debug Makefile target repeatedly invokes
gengowatcher.main with four separate "--set" calls which restarts Python and
reloads/saves AppConfig each time; change it to perform a single atomic
configuration update by either (1) extending the CLI to accept multiple "--set
<Section> <Key> <Value>" triples in one invocation and update the Makefile's
firefox-debug recipe to pass all four triples in one gengowatcher.main call, or
(2) add a new subcommand (e.g., configure-firefox-debug) in gengowatcher.main
that loads AppConfig once, sets browser_debug_url, browser_debug_browser_path,
browser_debug_profile_path and browser_debug_auto_launch, then saves; update the
Makefile to call that single subcommand. Ensure the unique targets/functions
affected are the Makefile target "firefox-debug" and the CLI entry point in
gengowatcher.main that handles "--set" (or the new "configure-firefox-debug"
handler) so the config write is done once atomically.
In `@tests/test_browser_debug_launcher.py`:
- Around line 14-29: Add negative tests alongside
test_get_firefox_debug_launch_spec_requires_local_ws_url to assert
get_firefox_debug_launch_spec rejects non-local or non-plain-ws endpoints: call
get_firefox_debug_launch_spec with examples like "ws://192.168.1.2:6000",
"wss://127.0.0.1:6000", and "ws://127.0.0.1:6000/session" (or any URL containing
"/session") and assert the returned spec is None (or raises the same failure
mode as the function uses). Reuse the same MagicMock config setup from the
existing test and add separate assertions for each invalid URL case to ensure
remote hosts, secure wss scheme, and session endpoints are rejected by
get_firefox_debug_launch_spec.
In `@tests/test_cli.py`:
- Around line 27-39: Add assertions to the
test_browser_session_flags_map_to_lightweight_commands test to also parse and
assert the suppressed alias flags map to the same attributes and lightweight
handling: call parser.parse_args with ["--sync-session"] and
["--check-session"], assert their sync_session_from_browser and
check_session_from_browser attributes are True, and assert
should_handle_lightweight_command(...) returns True for each; locate the parser
via build_argument_parser and the test function
test_browser_session_flags_map_to_lightweight_commands to add these one-line
checks so the hidden aliases in cli.py remain covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77aeb1ee-9606-40c2-99e2-23c3515a577a
📒 Files selected for processing (13)
Makefiledocs/websocket-contract.mdsrc/gengowatcher/browser_debug_launcher.pysrc/gengowatcher/browser_session.pysrc/gengowatcher/cli.pysrc/gengowatcher/config.pysrc/gengowatcher/main.pysrc/gengowatcher/watcher.pytests/test_browser_debug_launcher.pytests/test_browser_session.pytests/test_cli.pytests/test_watcher_comprehensive.pytests/test_websocket.py
Comment on lines
+59
to
+65
| def _looks_like_local_firefox_rdp_url(debug_url: str) -> bool: | ||
| parsed = urlparse(str(debug_url or "").strip()) | ||
| if parsed.scheme != "ws": | ||
| return False | ||
| if parsed.path.rstrip("/") == "/session": | ||
| return False | ||
| return (parsed.hostname or "127.0.0.1") in {"127.0.0.1", "localhost"} |
Contributor
There was a problem hiding this comment.
Edge case: URLs without a hostname are falsely classified as local.
urlparse("ws:///session").hostname is None, and (None or "127.0.0.1") makes this return True. Intended or not, it means a malformed debug URL silently passes the "is local" gate and flows into auto-launch/profile-write logic. Safer to require a real hostname.
🔧 Proposed fix
- return (parsed.hostname or "127.0.0.1") in {"127.0.0.1", "localhost"}
+ return parsed.hostname in {"127.0.0.1", "localhost"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gengowatcher/browser_debug_launcher.py` around lines 59 - 65, The helper
_looks_like_local_firefox_rdp_url currently treats a None hostname as
"127.0.0.1", which lets malformed URLs like "ws:///session" be classified as
local; update the function to require a real hostname by checking
parsed.hostname is not None and then testing parsed.hostname in {"127.0.0.1",
"localhost"} (instead of using (parsed.hostname or "127.0.0.1")), preserving the
existing scheme and path checks so only well-formed local hosts return True.
Comment on lines
+195
to
+212
| async def _probe_firefox_debug_server(debug_url: str) -> bool: | ||
| try: | ||
| async with websockets.connect(debug_url, max_size=1_000_000) as websocket: | ||
| raw_message = await asyncio.wait_for(websocket.recv(), timeout=2) | ||
| except Exception: | ||
| return False | ||
|
|
||
| try: | ||
| packet = json.loads(raw_message) | ||
| except json.JSONDecodeError: | ||
| return False | ||
| return isinstance(packet, dict) and packet.get("from") == "root" | ||
|
|
||
|
|
||
| def can_connect_to_firefox_debug_server(debug_url: str) -> bool: | ||
| if not _looks_like_local_firefox_rdp_url(debug_url): | ||
| return False | ||
| return bool(asyncio.run(_probe_firefox_debug_server(debug_url))) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether can_connect_to_firefox_debug_server / maybe_launch_managed_firefox_debug
# are ever invoked from async contexts (event loops) which would break asyncio.run().
rg -nP -C3 '\b(can_connect_to_firefox_debug_server|maybe_launch_managed_firefox_debug)\s*\('Repository: tdawe1/GengoWatcher
Length of output: 3173
🏁 Script executed:
# Check the context around line 635 in watcher.py
sed -n '600,650p' src/gengowatcher/watcher.py | head -60Repository: tdawe1/GengoWatcher
Length of output: 1972
🏁 Script executed:
# Check if the function containing line 635 is async
rg -nB 30 'maybe_launch_managed_firefox_debug\(' src/gengowatcher/watcher.py | grep -E '(async def|def )' | tail -1Repository: tdawe1/GengoWatcher
Length of output: 102
🏁 Script executed:
# Find the exact function signature at watcher.py line 635
awk 'NR>=600 && NR<=650' src/gengowatcher/watcher.pyRepository: tdawe1/GengoWatcher
Length of output: 1972
🏁 Script executed:
# Check websockets library usage and version constraints
rg -n 'websockets' src/gengowatcher/ pyproject.toml requirements.txt 2>/dev/null || trueRepository: tdawe1/GengoWatcher
Length of output: 2137
🏁 Script executed:
# Check if there are any async wrappers or context managers that might call _sync_session_from_browser
rg -nB 5 '_sync_session_from_browser' src/gengowatcher/Repository: tdawe1/GengoWatcher
Length of output: 2436
🏁 Script executed:
# Verify websockets library supports open_timeout parameter
# Check the browser_debug_launcher.py imports and how websockets is used
head -50 src/gengowatcher/browser_debug_launcher.pyRepository: tdawe1/GengoWatcher
Length of output: 1425
🌐 Web query:
websockets 16.0 python open_timeout parameter
💡 Result:
The open_timeout parameter in the Python websockets library version 16.0 controls the timeout for the opening handshake (TCP connection + HTTP upgrade to WebSocket) in seconds. The default value is 10 seconds. Setting it to None disables the timeout. It is available in: - Client APIs: - websockets.asyncio.client.connect(..., open_timeout=10) [59,8,19] - websockets.sync.client.connect(..., open_timeout=10) [11] - Server APIs: - websockets.asyncio.server.serve(..., open_timeout=10) [61] - websockets.sync.server.serve(..., open_timeout=10) [63] If the handshake doesn't complete within this time, a TimeoutError is raised. Example (asyncio client): import asyncio from websockets.asyncio.client import connect async def main: async with connect("ws://example.com", open_timeout=5.0) as ws: await ws.send("Hello") msg = await ws.recv print(msg) asyncio.run(main) [2,5 confirm v16.0 released Jan 10, 2026; parameter introduced in v10.0 and carried forward]
Citations:
- 1: https://websockets.readthedocs.io/en/stable/project/changelog.html
- 2: https://pypi.org/project/websockets/
- 3: https://websockets.readthedocs.io/en/15.0.1/reference/asyncio/client.html
- 4: https://websockets.readthedocs.io/en/stable/reference/sync/client.html
- 5: https://websockets.readthedocs.io/en/14.0/reference/asyncio/client.html
- 6: https://websockets.readthedocs.io/en/stable/reference/asyncio/client.html
- 7: https://websockets.readthedocs.io/en/14.1/reference/asyncio/server.html
- 8: https://websockets.readthedocs.io/en/14.0/reference/sync/server.html
Add a connect timeout to prevent handshake hangs.
The websockets.connect(debug_url, max_size=1_000_000) call lacks a timeout for the opening handshake. If the port accepts TCP but stalls on the WebSocket upgrade, the 2-second recv timeout won't help—it only applies after the connection succeeds. Add open_timeout=2 to bound the total wait.
🔧 Proposed fix
async def _probe_firefox_debug_server(debug_url: str) -> bool:
try:
- async with websockets.connect(debug_url, max_size=1_000_000) as websocket:
+ async with websockets.connect(
+ debug_url, max_size=1_000_000, open_timeout=2
+ ) as websocket:
raw_message = await asyncio.wait_for(websocket.recv(), timeout=2)
except Exception:
return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def _probe_firefox_debug_server(debug_url: str) -> bool: | |
| try: | |
| async with websockets.connect(debug_url, max_size=1_000_000) as websocket: | |
| raw_message = await asyncio.wait_for(websocket.recv(), timeout=2) | |
| except Exception: | |
| return False | |
| try: | |
| packet = json.loads(raw_message) | |
| except json.JSONDecodeError: | |
| return False | |
| return isinstance(packet, dict) and packet.get("from") == "root" | |
| def can_connect_to_firefox_debug_server(debug_url: str) -> bool: | |
| if not _looks_like_local_firefox_rdp_url(debug_url): | |
| return False | |
| return bool(asyncio.run(_probe_firefox_debug_server(debug_url))) | |
| async def _probe_firefox_debug_server(debug_url: str) -> bool: | |
| try: | |
| async with websockets.connect( | |
| debug_url, max_size=1_000_000, open_timeout=2 | |
| ) as websocket: | |
| raw_message = await asyncio.wait_for(websocket.recv(), timeout=2) | |
| except Exception: | |
| return False | |
| try: | |
| packet = json.loads(raw_message) | |
| except json.JSONDecodeError: | |
| return False | |
| return isinstance(packet, dict) and packet.get("from") == "root" | |
| def can_connect_to_firefox_debug_server(debug_url: str) -> bool: | |
| if not _looks_like_local_firefox_rdp_url(debug_url): | |
| return False | |
| return bool(asyncio.run(_probe_firefox_debug_server(debug_url))) |
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 199-199: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gengowatcher/browser_debug_launcher.py` around lines 195 - 212, The
websockets.connect call in _probe_firefox_debug_server can hang during the
opening handshake; update the websockets.connect(...) call inside
_probe_firefox_debug_server to include an open_timeout=2 (e.g.,
websockets.connect(debug_url, max_size=1_000_000, open_timeout=2)) so the
handshake is bounded while keeping the existing asyncio.wait_for recv timeout,
then leave can_connect_to_firefox_debug_server as-is to continue using
asyncio.run(_probe_firefox_debug_server(debug_url)).
Comment on lines
+157
to
+182
| try: | ||
| return main_module.fetch_browser_session_snapshot_sync(debug_url=debug_url) | ||
| except Exception: | ||
| if maybe_launch_managed_firefox_debug(config, debug_url): | ||
| timeout_sec, retry_interval_sec = get_firefox_debug_retry_window(config) | ||
| deadline = time.monotonic() + timeout_sec | ||
| last_exc: Exception | None = None | ||
| while time.monotonic() < deadline: | ||
| time.sleep(retry_interval_sec) | ||
| try: | ||
| return main_module.fetch_browser_session_snapshot_sync( | ||
| debug_url=debug_url | ||
| ) | ||
| except Exception as exc: # pragma: no cover - exercised via caller | ||
| last_exc = exc | ||
| if last_exc is not None: | ||
| raise last_exc | ||
| session_token = main_module.fetch_browser_session_token_sync( | ||
| debug_url=debug_url | ||
| ) | ||
| return BrowserSessionSnapshot( | ||
| session_token=session_token, | ||
| user_key=FALLBACK_BROWSER_USER_KEY, | ||
| user_agent=FALLBACK_BROWSER_USER_AGENT, | ||
| accept_language=FALLBACK_BROWSER_ACCEPT_LANGUAGE, | ||
| ) |
Contributor
There was a problem hiding this comment.
Preserve existing config values on token-only fallback.
If the snapshot fetch fails, this path fabricates browser-user-key / Helium Browser and _sync_session_from_browser persists them, which can corrupt the user’s WebSocket and Network settings. Keep existing values, and narrow the catch to expected browser attach failures.
Proposed fix
session_token = main_module.fetch_browser_session_token_sync(
debug_url=debug_url
)
return BrowserSessionSnapshot(
session_token=session_token,
- user_key=FALLBACK_BROWSER_USER_KEY,
- user_agent=FALLBACK_BROWSER_USER_AGENT,
- accept_language=FALLBACK_BROWSER_ACCEPT_LANGUAGE,
+ user_key=str(config.get("WebSocket", "user_key") or "").strip(),
+ user_agent=str(config.get("Network", "browser_user_agent") or "").strip(),
+ accept_language=str(
+ config.get("Network", "browser_accept_language") or ""
+ ).strip(),
)As per coding guidelines, "Catch specific exceptions where the failure mode is known; use broad except Exception blocks only at application boundaries, startup paths, UI loops, and optional integrations" and "Avoid silent exception swallowing unless the code intentionally degrades gracefully".
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 159-159: Do not catch blind exception: Exception
(BLE001)
[warning] 170-170: Do not catch blind exception: Exception
(BLE001)
[warning] 173-173: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gengowatcher/cli.py` around lines 157 - 182, The broad except Exception
around fetch_browser_session_snapshot_sync should be narrowed to only catch the
expected browser-attach/timeout exception (e.g., BrowserAttachError or whatever
specific exception main_module raises) and re-raise any other exceptions; when
falling back to a token-only BrowserSessionSnapshot (created via
fetch_browser_session_token_sync) do not fabricate default
user_key/user_agent/accept_language—retrieve and reuse the existing values from
the runtime/config (e.g., read from config or a current-session accessor on
main_module) and only use FALLBACK_* constants as a last-resort fallback; keep
references to fetch_browser_session_snapshot_sync,
maybe_launch_managed_firefox_debug, fetch_browser_session_token_sync,
BrowserSessionSnapshot, and FALLBACK_BROWSER_* when implementing this change.
Comment on lines
+13
to
+16
| from .browser_session import ( | ||
| fetch_browser_session_snapshot_sync, | ||
| fetch_browser_session_token_sync, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether the two new imports are actually referenced anywhere in main.py
rg -nP '\b(fetch_browser_session_snapshot_sync|fetch_browser_session_token_sync)\b' src/gengowatcher/main.pyRepository: tdawe1/GengoWatcher
Length of output: 147
🏁 Script executed:
#!/bin/bash
# Check if these symbols are used in runtime.py
rg -n '\b(fetch_browser_session_snapshot_sync|fetch_browser_session_token_sync)\b' src/gengowatcher/runtime.py
echo "---"
# Check the structure of main.py around the suspected dead code
wc -l src/gengowatcher/main.pyRepository: tdawe1/GengoWatcher
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Read main.py to understand the structure and whether the imports are used
cat -n src/gengowatcher/main.pyRepository: tdawe1/GengoWatcher
Length of output: 8948
🏁 Script executed:
#!/bin/bash
# Search the entire codebase for uses of these symbols (excluding import lines)
rg -n 'fetch_browser_session_snapshot_sync|fetch_browser_session_token_sync' src/gengowatcher/ --type py | grep -v 'from .browser_session import' | grep -v '^src/gengowatcher/browser_session.py'Repository: tdawe1/GengoWatcher
Length of output: 916
🏁 Script executed:
#!/bin/bash
# Check how cli.py imports main and uses main_module
head -n 40 src/gengowatcher/cli.py | cat -n
echo "---"
# Find the specific lines in cli.py that reference main_module
rg -B2 -A2 'main_module\.' src/gengowatcher/cli.py | head -n 50Repository: tdawe1/GengoWatcher
Length of output: 2223
🏁 Script executed:
#!/bin/bash
# Search for where main_module is imported in cli.py
rg -n 'main_module' src/gengowatcher/cli.py | head -n 20
echo "---"
# Look for import statements in cli.py that bring in main
rg -n 'import.*main|from.*main' src/gengowatcher/cli.pyRepository: tdawe1/GengoWatcher
Length of output: 392
Remove these imports from main.py and import them directly in cli.py instead.
These symbols are not used within main.py itself; they're only re-exported for cli.py to access via main_module. This creates unnecessary coupling and violates the guideline to keep CLI concerns in cli.py. Move the imports directly to cli.py at the point where they're needed (around line 154), importing from browser_session rather than routing through main_module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gengowatcher/main.py` around lines 13 - 16, Remove the unused re-exports
from main.py: delete the imports fetch_browser_session_snapshot_sync and
fetch_browser_session_token_sync from main_module and instead import them
directly where needed in cli.py; update cli.py to import
fetch_browser_session_snapshot_sync and fetch_browser_session_token_sync from
the browser_session module at the point they are used (rather than accessing
them via main_module) so CLI concerns stay in cli.py and main_module has no
unnecessary coupling.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/gengowatcher/browser_debug_launcher.py (2)
62-73:⚠️ Potential issue | 🟡 MinorReject malformed local debug URLs before deriving the port.
Line 68 still treats missing hostnames as localhost, so malformed values such as
ws:///debugcan enter the managed-launch path. Require an explicit local hostname before returningTrue.Proposed fix
def _looks_like_local_firefox_rdp_url(debug_url: str) -> bool: parsed = urlparse(str(debug_url or "").strip()) if parsed.scheme != "ws": return False if parsed.path.rstrip("/") == "/session": return False - return (parsed.hostname or "127.0.0.1") in {"127.0.0.1", "localhost"} + return parsed.hostname in {"127.0.0.1", "localhost"}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gengowatcher/browser_debug_launcher.py` around lines 62 - 73, The predicate _looks_like_local_firefox_rdp_url currently treats a missing hostname as localhost (parsed.hostname or "127.0.0.1"), allowing malformed URLs like "ws:///debug" to pass; update _looks_like_local_firefox_rdp_url to require parsed.hostname to be present and equal to "127.0.0.1" or "localhost" (do not substitute a default), and return False when parsed.hostname is None or empty. Also audit _firefox_debug_port (function name _firefox_debug_port) usage to ensure callers only reach it for well-formed URLs (or adjust it to raise/propagate an error when parsed.port is None instead of silently defaulting) so malformed URLs are rejected before deriving a port.
286-290:⚠️ Potential issue | 🟡 MinorBound the WebSocket opening handshake.
The 2-second
recvtimeout only applies after the connection is established. Addopen_timeout=2so a stalled WebSocket upgrade cannot hang this probe.Proposed fix
async def _probe_firefox_debug_server(debug_url: str) -> bool: try: - async with websockets.connect(debug_url, max_size=1_000_000) as websocket: + async with websockets.connect( + debug_url, + max_size=1_000_000, + open_timeout=2, + ) as websocket: raw_message = await asyncio.wait_for(websocket.recv(), timeout=2)Please verify the installed
websocketsversion supports this parameter:Does the Python websockets client connect API support the open_timeout parameter, and in which versions?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gengowatcher/browser_debug_launcher.py` around lines 286 - 290, The probe currently only applies asyncio.wait_for to websocket.recv, letting a stalled WebSocket upgrade hang; update the websockets.connect call inside _probe_firefox_debug_server to pass open_timeout=2 so the opening handshake is bounded (keep existing recv timeout logic intact and preserve the current exception handling path), and before landing the change verify the installed websockets client supports the open_timeout parameter by checking the package docs or inspecting the connect() signature/installed websockets.__version__ to ensure compatibility.src/gengowatcher/cli.py (1)
152-183:⚠️ Potential issue | 🟠 MajorDo not overwrite existing browser metadata on token-only fallback.
If snapshot loading fails, this fallback fabricates
user_key,user_agent, andaccept_language;_sync_session_from_browserthen persists those values. Preserve the current config values and only fall back to constants as a last resort.Proposed fix
session_token = main_module.fetch_browser_session_token_sync( debug_url=debug_url ) return BrowserSessionSnapshot( session_token=session_token, - user_key=FALLBACK_BROWSER_USER_KEY, - user_agent=FALLBACK_BROWSER_USER_AGENT, - accept_language=FALLBACK_BROWSER_ACCEPT_LANGUAGE, + user_key=str( + config.get("WebSocket", "user_key") or FALLBACK_BROWSER_USER_KEY + ).strip(), + user_agent=str( + config.get("Network", "browser_user_agent") + or FALLBACK_BROWSER_USER_AGENT + ).strip(), + accept_language=str( + config.get("Network", "browser_accept_language") + or FALLBACK_BROWSER_ACCEPT_LANGUAGE + ).strip(), )Also narrow the broad
except Exceptionto the specific browser attach failure type if one is available. As per coding guidelines, “Catch specific exceptions where the failure mode is known” and “Avoid silent exception swallowing unless the code intentionally degrades gracefully.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gengowatcher/cli.py` around lines 152 - 183, The current _load_browser_session_snapshot swallows all Exceptions and on token-only fallback fabricates user_key/user_agent/accept_language which can overwrite real config; change the broad "except Exception" around fetch_browser_session_snapshot_sync to catch the specific browser attach failure exception (e.g. BrowserAttachError or the specific exception provided by main_module) and only fall back to starting managed Firefox and retries on that error, and when finally creating the token-only BrowserSessionSnapshot use existing config values (e.g. config.browser.user_key, config.browser.user_agent, config.browser.accept_language or the equivalent fields on AppConfig) if present, falling back to FALLBACK_BROWSER_USER_KEY / FALLBACK_BROWSER_USER_AGENT / FALLBACK_BROWSER_ACCEPT_LANGUAGE only when the config values are missing; ensure you still return the token from main_module.fetch_browser_session_token_sync and do not persist the fabricated constants as the authoritative config.
🧹 Nitpick comments (1)
src/gengowatcher/browser_debug_launcher.py (1)
196-239: Persist profile preference files atomically.Line 239 writes
user.js/prefs.jsin place. If the process is interrupted mid-write, the managed Firefox profile can be left with truncated preferences.Proposed fix
- path.write_text(rendered, encoding="utf-8") + tmp_path = path.with_name(f"{path.name}.tmp") + tmp_path.write_text(rendered, encoding="utf-8") + tmp_path.replace(path)As per coding guidelines, “Use atomic write patterns for state and config persistence rather than in-place file mutation.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gengowatcher/browser_debug_launcher.py` around lines 196 - 239, _upsert_firefox_pref_file currently writes prefs in-place which can leave truncated files if interrupted; change it to perform an atomic write by writing the rendered content to a temporary file in the same directory (eg. path.parent), flush and fsync the file descriptor, close it, then atomically replace the original with os.replace (or pathlib.Path.replace) to swap the temp into place; ensure you preserve encoding/permissions as needed and still create the file when none existed (use the same rendered string you currently write) before returning from _upsert_firefox_pref_file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_browser_debug_launcher.py`:
- Around line 250-255: Replace the try/except that uses assert False with
pytest's exception context: use pytest.raises(RuntimeError) as excinfo around
the call to launch_managed_firefox_debug(spec), then assert the expected message
is in str(excinfo.value); this change should be made in the test function in
tests/test_browser_debug_launcher.py where subprocess.Popen is patched and
launch_managed_firefox_debug is invoked.
---
Duplicate comments:
In `@src/gengowatcher/browser_debug_launcher.py`:
- Around line 62-73: The predicate _looks_like_local_firefox_rdp_url currently
treats a missing hostname as localhost (parsed.hostname or "127.0.0.1"),
allowing malformed URLs like "ws:///debug" to pass; update
_looks_like_local_firefox_rdp_url to require parsed.hostname to be present and
equal to "127.0.0.1" or "localhost" (do not substitute a default), and return
False when parsed.hostname is None or empty. Also audit _firefox_debug_port
(function name _firefox_debug_port) usage to ensure callers only reach it for
well-formed URLs (or adjust it to raise/propagate an error when parsed.port is
None instead of silently defaulting) so malformed URLs are rejected before
deriving a port.
- Around line 286-290: The probe currently only applies asyncio.wait_for to
websocket.recv, letting a stalled WebSocket upgrade hang; update the
websockets.connect call inside _probe_firefox_debug_server to pass
open_timeout=2 so the opening handshake is bounded (keep existing recv timeout
logic intact and preserve the current exception handling path), and before
landing the change verify the installed websockets client supports the
open_timeout parameter by checking the package docs or inspecting the connect()
signature/installed websockets.__version__ to ensure compatibility.
In `@src/gengowatcher/cli.py`:
- Around line 152-183: The current _load_browser_session_snapshot swallows all
Exceptions and on token-only fallback fabricates
user_key/user_agent/accept_language which can overwrite real config; change the
broad "except Exception" around fetch_browser_session_snapshot_sync to catch the
specific browser attach failure exception (e.g. BrowserAttachError or the
specific exception provided by main_module) and only fall back to starting
managed Firefox and retries on that error, and when finally creating the
token-only BrowserSessionSnapshot use existing config values (e.g.
config.browser.user_key, config.browser.user_agent,
config.browser.accept_language or the equivalent fields on AppConfig) if
present, falling back to FALLBACK_BROWSER_USER_KEY / FALLBACK_BROWSER_USER_AGENT
/ FALLBACK_BROWSER_ACCEPT_LANGUAGE only when the config values are missing;
ensure you still return the token from
main_module.fetch_browser_session_token_sync and do not persist the fabricated
constants as the authoritative config.
---
Nitpick comments:
In `@src/gengowatcher/browser_debug_launcher.py`:
- Around line 196-239: _upsert_firefox_pref_file currently writes prefs in-place
which can leave truncated files if interrupted; change it to perform an atomic
write by writing the rendered content to a temporary file in the same directory
(eg. path.parent), flush and fsync the file descriptor, close it, then
atomically replace the original with os.replace (or pathlib.Path.replace) to
swap the temp into place; ensure you preserve encoding/permissions as needed and
still create the file when none existed (use the same rendered string you
currently write) before returning from _upsert_firefox_pref_file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 485a59d1-b452-43c2-af91-d25aa71a5ad7
📒 Files selected for processing (7)
.gitignoreMakefilesrc/gengowatcher/browser_debug_launcher.pysrc/gengowatcher/cli.pysrc/gengowatcher/config.pysrc/gengowatcher/main.pytests/test_browser_debug_launcher.py
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- src/gengowatcher/config.py
Comment on lines
+250
to
+255
| with patch("gengowatcher.browser_debug_launcher.subprocess.Popen") as mock_popen: | ||
| try: | ||
| launch_managed_firefox_debug(spec) | ||
| assert False, "Expected locked Firefox build to be rejected" | ||
| except RuntimeError as exc: | ||
| assert "disables DevTools remote debugging" in str(exc) |
Contributor
There was a problem hiding this comment.
Use pytest.raises instead of assert False.
assert False is removed under Python optimisation; use pytest’s exception assertion directly.
Proposed fix
from pathlib import Path
from unittest.mock import MagicMock, patch
+import pytest
+
from gengowatcher.browser_debug_launcher import ( with patch("gengowatcher.browser_debug_launcher.subprocess.Popen") as mock_popen:
- try:
+ with pytest.raises(
+ RuntimeError,
+ match="disables DevTools remote debugging",
+ ):
launch_managed_firefox_debug(spec)
- assert False, "Expected locked Firefox build to be rejected"
- except RuntimeError as exc:
- assert "disables DevTools remote debugging" in str(exc)
mock_popen.assert_not_called()🧰 Tools
🪛 Ruff (0.15.10)
[warning] 253-253: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_browser_debug_launcher.py` around lines 250 - 255, Replace the
try/except that uses assert False with pytest's exception context: use
pytest.raises(RuntimeError) as excinfo around the call to
launch_managed_firefox_debug(spec), then assert the expected message is in
str(excinfo.value); this change should be made in the test function in
tests/test_browser_debug_launcher.py where subprocess.Popen is patched and
launch_managed_firefox_debug is invoked.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/gengowatcher/browser_debug_launcher.py (2)
64-75:⚠️ Potential issue | 🟡 MinorRequire a real local host and explicit port.
Line 70 still treats malformed URLs such as
ws:///debugas local, andws://127.0.0.1passes even though Firefox is launched on port 6000 while probes use the unnormalised URL. Reject missing hosts and ports at the gate.Proposed fix
def _looks_like_local_firefox_rdp_url(debug_url: str) -> bool: parsed = urlparse(str(debug_url or "").strip()) if parsed.scheme != "ws": return False if parsed.path.rstrip("/") == "/session": return False - return (parsed.hostname or "127.0.0.1") in {"127.0.0.1", "localhost"} + if parsed.hostname not in {"127.0.0.1", "localhost"}: + return False + return parsed.port is not NoneVerification:
#!/bin/bash # Description: Show hostname/port parsing for malformed and no-port ws URLs. # Expected: hostname is None for ws:///debug; port is None for ws://127.0.0.1. python - <<'PY' from urllib.parse import urlparse for url in ("ws:///debug", "ws://127.0.0.1", "ws://127.0.0.1:6000"): parsed = urlparse(url) print(url, "hostname=", parsed.hostname, "port=", parsed.port) PY
303-306:⚠️ Potential issue | 🟡 MinorBound the WebSocket opening handshake.
The probe’s
recv()timeout only starts after the WebSocket opens. With websockets’ default opening timeout, one stalled handshake can exceed the configured retry window; set an explicitopen_timeout.Proposed fix
async def _probe_firefox_debug_server(debug_url: str) -> bool: try: - async with websockets.connect(debug_url, max_size=1_000_000) as websocket: + async with websockets.connect( + debug_url, + max_size=1_000_000, + open_timeout=2, + ) as websocket: raw_message = await asyncio.wait_for(websocket.recv(), timeout=2)Verification:
Does websockets 16.0 websockets.connect support the open_timeout parameter?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gengowatcher/browser_debug_launcher.py` around lines 303 - 306, The WebSocket opening handshake can stall because the current timeout only applies to recv(); in _probe_firefox_debug_server call websockets.connect with an explicit open_timeout (e.g. open_timeout=2 or match your recv timeout) so the connect will fail fast and respect the probe retry window; update the websockets.connect invocation to include open_timeout and ensure your runtime uses a websockets version that supports the open_timeout parameter.tests/test_browser_debug_launcher.py (1)
292-297:⚠️ Potential issue | 🟡 MinorUse
pytest.raisesinstead ofassert False.Line 295 can be stripped under Python optimisation, so the failure assertion may disappear. Use pytest’s exception assertion directly.
Proposed fix
from pathlib import Path from unittest.mock import MagicMock, patch +import pytest + from gengowatcher.browser_debug_launcher import (with patch("gengowatcher.browser_debug_launcher.subprocess.Popen") as mock_popen: - try: + with pytest.raises( + RuntimeError, + match="disables DevTools remote debugging", + ): launch_managed_firefox_debug(spec) - assert False, "Expected locked Firefox build to be rejected" - except RuntimeError as exc: - assert "disables DevTools remote debugging" in str(exc) mock_popen.assert_not_called()Verification:
#!/bin/bash # Description: Demonstrate that Python removes assert statements under optimisation. # Expected: prints the final line instead of raising AssertionError. python -O - <<'PY' assert False, "this assertion is removed under -O" print("assertions are disabled under -O") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_browser_debug_launcher.py` around lines 292 - 297, Replace the try/except/assert pattern with pytest's exception context: use pytest.raises(RuntimeError) to call launch_managed_firefox_debug(spec) (the block currently wrapped by patch("gengowatcher.browser_debug_launcher.subprocess.Popen") as mock_popen) and assert the exception message contains "disables DevTools remote debugging"; also ensure pytest is imported in the test file if not already. This removes the fragile assert False and makes the expectation explicit and safe under Python optimization.
🧹 Nitpick comments (1)
src/gengowatcher/config.py (1)
460-501: Remove the duplicate TOML serializer.The
_serialize_toml_valuedefined on Line 460 is overwritten by the second definition on Line 610, and the fallback block after Line 642 is unreachable. Keep a single implementation so_dump_toml()has one clear code path.Proposed fix
- `@staticmethod` - def _serialize_toml_value(value: Any) -> str: - """Serialize Python values to TOML literals.""" - if isinstance(value, bool): - return "true" if value else "false" - if isinstance(value, int): - return str(value) - if isinstance(value, float): - return repr(value) - if isinstance(value, str): - escaped = ( - value.replace("\\", "\\\\").replace('"', '\\"').replace("\n", "\\n") - ) - return f'"{escaped}"' - if isinstance(value, list): - return ( - "[" - + ", ".join(AppConfig._serialize_toml_value(item) for item in value) - + "]" - ) - if isinstance(value, dict): - items = ", ".join( - f"{key} = {AppConfig._serialize_toml_value(item)}" - for key, item in value.items() - ) - return "{ " + items + " }" - if value is None: - return '""' - return json.dumps(value) - `@classmethod` def _dump_toml(cls, data: Dict[str, Dict[str, Any]]) -> str: """Serialize the nested config dictionary to a TOML document."""if value is None: return '""' - if isinstance(value, bool): - return "true" if value else "false" - if isinstance(value, (int, float)): - return str(value) return json.dumps(value) - - try: - return json.dumps(value) - except (TypeError, ValueError): - pass - - return str(value)Also applies to: 610-649
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gengowatcher/config.py` around lines 460 - 501, There are two definitions of _serialize_toml_value causing the second to overwrite the first and leaving the fallback unreachable; remove the duplicate so only one implementation remains (the one that supports bool, int, float, str escaping, list/dict recursion, None handling, and final json.dumps fallback) and ensure _dump_toml continues to call cls._serialize_toml_value; update/remove the second definition (the one around the later lines) and keep a single comprehensive _serialize_toml_value implementation referenced by _dump_toml to guarantee a single clear code path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 40: Always invoke the gengowatcher config set so an empty
FIREFOX_DEBUG_SEED_PROFILE clears the persisted value: remove the conditional
that only runs the PYTHON command when "$(FIREFOX_DEBUG_SEED_PROFILE)" is
non-empty and instead always run PYTHONPATH=src $(PYTHON) -m gengowatcher.main
--set WebSocket browser_debug_seed_profile_path "$(FIREFOX_DEBUG_SEED_PROFILE)";
this ensures the persistent key is overwritten (cleared) when the Make variable
is empty.
In `@src/gengowatcher/browser_session.py`:
- Around line 920-936: The current RDP flow calls _firefox_rdp_evaluate_json
with _firefox_page_state_expression() and if that raises BrowserSessionError it
aborts before fetching cookies; wrap the page-state evaluation in a try/except
that catches BrowserSessionError, sets page_state = {} on failure, and then
always proceeds to call _firefox_rdp_evaluate_json for the cookie lookup (using
_firefox_rdp_get_browser_console_actor and _firefox_cookie_lookup_expression())
so session cookies are fetched even when page-state evaluation fails; do not
swallow errors from the cookie lookup itself—only fallback page_state to {} on
BrowserSessionError from _firefox_page_state_expression().
---
Duplicate comments:
In `@src/gengowatcher/browser_debug_launcher.py`:
- Around line 303-306: The WebSocket opening handshake can stall because the
current timeout only applies to recv(); in _probe_firefox_debug_server call
websockets.connect with an explicit open_timeout (e.g. open_timeout=2 or match
your recv timeout) so the connect will fail fast and respect the probe retry
window; update the websockets.connect invocation to include open_timeout and
ensure your runtime uses a websockets version that supports the open_timeout
parameter.
In `@tests/test_browser_debug_launcher.py`:
- Around line 292-297: Replace the try/except/assert pattern with pytest's
exception context: use pytest.raises(RuntimeError) to call
launch_managed_firefox_debug(spec) (the block currently wrapped by
patch("gengowatcher.browser_debug_launcher.subprocess.Popen") as mock_popen) and
assert the exception message contains "disables DevTools remote debugging"; also
ensure pytest is imported in the test file if not already. This removes the
fragile assert False and makes the expectation explicit and safe under Python
optimization.
---
Nitpick comments:
In `@src/gengowatcher/config.py`:
- Around line 460-501: There are two definitions of _serialize_toml_value
causing the second to overwrite the first and leaving the fallback unreachable;
remove the duplicate so only one implementation remains (the one that supports
bool, int, float, str escaping, list/dict recursion, None handling, and final
json.dumps fallback) and ensure _dump_toml continues to call
cls._serialize_toml_value; update/remove the second definition (the one around
the later lines) and keep a single comprehensive _serialize_toml_value
implementation referenced by _dump_toml to guarantee a single clear code path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dac1ebe4-abb2-467f-bc8a-3d054e5377e1
📒 Files selected for processing (7)
Makefilesrc/gengowatcher/browser_debug_launcher.pysrc/gengowatcher/browser_session.pysrc/gengowatcher/config.pytests/test_browser_debug_launcher.pytests/test_browser_session.pytests/test_config.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_config.py
| PYTHONPATH=src $(PYTHON) -m gengowatcher.main --set WebSocket browser_debug_url "$(FIREFOX_DEBUG_URL)" | ||
| PYTHONPATH=src $(PYTHON) -m gengowatcher.main --set Paths browser_debug_browser_path "$(FIREFOX_DEBUG_BROWSER)" | ||
| PYTHONPATH=src $(PYTHON) -m gengowatcher.main --set WebSocket browser_debug_profile_path "$(FIREFOX_DEBUG_PROFILE)" | ||
| if [ -n "$(FIREFOX_DEBUG_SEED_PROFILE)" ]; then PYTHONPATH=src $(PYTHON) -m gengowatcher.main --set WebSocket browser_debug_seed_profile_path "$(FIREFOX_DEBUG_SEED_PROFILE)"; fi |
Contributor
There was a problem hiding this comment.
Clear the persisted seed profile when the Make variable is empty.
Line 40 skips the --set call when FIREFOX_DEBUG_SEED_PROFILE is empty, so a previously saved seed profile remains active and can be reused unexpectedly. Set it every time so the default empty value actually clears the config.
Proposed fix
- if [ -n "$(FIREFOX_DEBUG_SEED_PROFILE)" ]; then PYTHONPATH=src $(PYTHON) -m gengowatcher.main --set WebSocket browser_debug_seed_profile_path "$(FIREFOX_DEBUG_SEED_PROFILE)"; fi
+ PYTHONPATH=src $(PYTHON) -m gengowatcher.main --set WebSocket browser_debug_seed_profile_path "$(FIREFOX_DEBUG_SEED_PROFILE)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ -n "$(FIREFOX_DEBUG_SEED_PROFILE)" ]; then PYTHONPATH=src $(PYTHON) -m gengowatcher.main --set WebSocket browser_debug_seed_profile_path "$(FIREFOX_DEBUG_SEED_PROFILE)"; fi | |
| PYTHONPATH=src $(PYTHON) -m gengowatcher.main --set WebSocket browser_debug_seed_profile_path "$(FIREFOX_DEBUG_SEED_PROFILE)" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 40, Always invoke the gengowatcher config set so an empty
FIREFOX_DEBUG_SEED_PROFILE clears the persisted value: remove the conditional
that only runs the PYTHON command when "$(FIREFOX_DEBUG_SEED_PROFILE)" is
non-empty and instead always run PYTHONPATH=src $(PYTHON) -m gengowatcher.main
--set WebSocket browser_debug_seed_profile_path "$(FIREFOX_DEBUG_SEED_PROFILE)";
this ensures the persistent key is overwritten (cleared) when the Make variable
is empty.
Comment on lines
+920
to
+936
| client = await _open_firefox_rdp_client(debug_url) | ||
| try: | ||
| tab, _tabs_response = await _firefox_rdp_get_gengo_tab( | ||
| client, | ||
| preferred_url_fragments=(GENGO_REALTIME_PATH, GENGO_SUMMARY_PATH), | ||
| ) | ||
| page_state = await _firefox_rdp_evaluate_json( | ||
| client, | ||
| _firefox_tab_console_actor(tab), | ||
| _firefox_page_state_expression(), | ||
| inner_window_id=tab.get("innerWindowId"), | ||
| ) | ||
| cookie_state = await _firefox_rdp_evaluate_json( | ||
| client, | ||
| await _firefox_rdp_get_browser_console_actor(client), | ||
| _firefox_cookie_lookup_expression(), | ||
| ) |
Contributor
There was a problem hiding this comment.
Keep Firefox RDP cookie sync working when page-state evaluation fails.
Unlike the CDP and BiDi paths, this RDP path lets a BrowserSessionError from _firefox_page_state_expression() abort before the cookie lookup. Fall back to {} for page metadata and still fetch the session cookie.
Proposed fix
- page_state = await _firefox_rdp_evaluate_json(
- client,
- _firefox_tab_console_actor(tab),
- _firefox_page_state_expression(),
- inner_window_id=tab.get("innerWindowId"),
- )
+ try:
+ page_state = await _firefox_rdp_evaluate_json(
+ client,
+ _firefox_tab_console_actor(tab),
+ _firefox_page_state_expression(),
+ inner_window_id=tab.get("innerWindowId"),
+ )
+ except BrowserSessionError:
+ page_state = {}
cookie_state = await _firefox_rdp_evaluate_json(
client,
await _firefox_rdp_get_browser_console_actor(client),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/gengowatcher/browser_session.py` around lines 920 - 936, The current RDP
flow calls _firefox_rdp_evaluate_json with _firefox_page_state_expression() and
if that raises BrowserSessionError it aborts before fetching cookies; wrap the
page-state evaluation in a try/except that catches BrowserSessionError, sets
page_state = {} on failure, and then always proceeds to call
_firefox_rdp_evaluate_json for the cookie lookup (using
_firefox_rdp_get_browser_console_actor and _firefox_cookie_lookup_expression())
so session cookies are fetched even when page-state evaluation fails; do not
swallow errors from the cookie lookup itself—only fallback page_state to {} on
BrowserSessionError from _firefox_page_state_expression().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds Firefox support to the live browser-session sync path using Firefox's DevTools remote debugging protocol instead of WebDriver/BiDi semantics, and it adds a managed launcher so the watcher can recover when a local Firefox debug endpoint is not already running.
Before this change, the browser-session sync path assumed Chromium-style CDP or manual browser preparation. That meant the session token sync, browser activity refresh, and related runtime recovery flows could not operate against a Firefox DevTools websocket endpoint, and users had to start and maintain the debug browser session themselves.
The root issue was that the browser-session layer treated browser debugging as a single transport shape. Firefox needs a distinct attach path for tab discovery, cookie lookup, page-state evaluation, and navigation nudges. On top of that, the watcher had no dedicated mechanism to launch a local Firefox debug profile when attachment failed.
This PR fixes that by introducing a Firefox RDP backend alongside the existing browser-session logic, then layering a small managed launcher on top for local Firefox debug endpoints. The launcher writes a dedicated Firefox profile with the required devtools prefs, starts Firefox with
--start-debugger-server, and exposes a new--start-firefox-debuglightweight command. The watcher and CLI session-sync flows now retry by launching that managed Firefox instance when a configured localws://endpoint cannot be reached. Amake firefox-debugtarget was also added so this can be bootstrapped from the repo's normal workflow.The user impact is that Firefox can now be used for the browser-session sync and page activity path with a devtools-style attach model, and the tool can bring up a dedicated Firefox debug session on demand instead of failing immediately when the endpoint is missing.
Validation used:
python3 -m py_compile src/gengowatcher/main.py src/gengowatcher/browser_debug_launcher.py src/gengowatcher/cli.py src/gengowatcher/watcher.py src/gengowatcher/config.py tests/test_browser_debug_launcher.py tests/test_cli.py tests/test_browser_session.py tests/test_watcher_comprehensive.pypython3 -m pytest tests/test_browser_debug_launcher.py tests/test_cli.py tests/test_browser_session.py tests/test_watcher_comprehensive.py -q