Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 104 additions & 2 deletions src/notebooklm/cli/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import json
import logging
import os
import shutil
import subprocess
import sys
import time
Expand All @@ -35,6 +36,7 @@
from ..paths import (
get_browser_profile_dir,
get_context_path,
get_home_dir,
get_path_info,
get_storage_path,
)
Expand Down Expand Up @@ -340,6 +342,45 @@ def _ensure_chromium_installed() -> None:
)


def _clear_auth_files(storage_path: Path, browser_profile: Path) -> list[Path]:
"""Delete auth files/directories for the active profile.

Also checks legacy default-profile paths when the resolved paths are profile-based.
Returns the paths that were actually removed.
"""
paths_to_remove = [storage_path, browser_profile]

legacy_storage = get_home_dir() / "storage_state.json"
legacy_browser = get_home_dir() / "browser_profile"
for legacy_path in (legacy_storage, legacy_browser):
if legacy_path not in paths_to_remove:
paths_to_remove.append(legacy_path)
Comment on lines +345 to +357
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Limit legacy cleanup to the default profile.

_clear_auth_files() always appends the legacy home-root auth paths, so login --fresh / auth logout on a non-default profile also wipes the legacy default profile's session. That breaks profile isolation while the command text says it only affects the active profile.

Proposed fix
-def _clear_auth_files(storage_path: Path, browser_profile: Path) -> list[Path]:
+def _clear_auth_files(
+    storage_path: Path, browser_profile: Path, *, clear_legacy_default: bool = False
+) -> list[Path]:
     """Delete auth files/directories for the active profile.

     Also checks legacy default-profile paths when the resolved paths are profile-based.
     Returns the paths that were actually removed.
     """
     paths_to_remove = [storage_path, browser_profile]

-    legacy_storage = get_home_dir() / "storage_state.json"
-    legacy_browser = get_home_dir() / "browser_profile"
-    for legacy_path in (legacy_storage, legacy_browser):
-        if legacy_path not in paths_to_remove:
-            paths_to_remove.append(legacy_path)
+    if clear_legacy_default:
+        home_dir = get_home_dir()
+        for legacy_path in (home_dir / "storage_state.json", home_dir / "browser_profile"):
+            if legacy_path not in paths_to_remove:
+                paths_to_remove.append(legacy_path)

Update the two call sites to pass clear_legacy_default=True only when the resolved profile is "default".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/notebooklm/cli/session.py` around lines 345 - 357, _clear_auth_files
currently always appends legacy home-root auth paths (legacy_storage and
legacy_browser) which causes logout/--fresh on non-default profiles to wipe the
default profile; change _clear_auth_files(signature) to accept a boolean
parameter clear_legacy_default (default False) and only append legacy_storage
and legacy_browser when clear_legacy_default is True, then update the two call
sites that invoke _clear_auth_files to pass clear_legacy_default=True only when
the resolved profile name equals "default" so legacy cleanup is limited to the
default profile.


removed: list[Path] = []
for path in paths_to_remove:
if not path.exists():
continue
if path.is_symlink():
path.unlink()
elif path.is_dir():
shutil.rmtree(path)
else:
path.unlink(missing_ok=True)
Comment on lines +360 to +368
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Handle broken symlinks before the exists() guard.

A dangling symlink makes Path.exists() return False, so this loop skips it instead of unlinking it. That leaves stale storage_state.json / browser_profile entries behind, and the later login setup can fail with a pre-existing path.

Proposed fix
     removed: list[Path] = []
     for path in paths_to_remove:
-        if not path.exists():
-            continue
         if path.is_symlink():
             path.unlink()
+            removed.append(path)
+            continue
+        if not path.exists():
+            continue
         elif path.is_dir():
             shutil.rmtree(path)
         else:
             path.unlink(missing_ok=True)
         removed.append(path)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/notebooklm/cli/session.py` around lines 360 - 368, The loop over
paths_to_remove incorrectly skips broken symlinks because Path.exists() is False
for dangling links; change the existence guard to detect symlinks too (e.g., use
path.lexists() or test (path.exists() or path.is_symlink())) so dangling
symlinks are unlinked; update the loop around paths_to_remove (the block that
calls path.unlink(), shutil.rmtree(path), and path.unlink(missing_ok=True)) to
first consider path.is_symlink() or use lexists() before continuing.

removed.append(path)

return removed


def _get_recovered_page(context: Any, current_page: Any) -> Any:
"""Recover a live page when the original Playwright page was closed."""
pages = [page for page in context.pages if not page.is_closed()]
if pages:
return pages[0]
if current_page and not current_page.is_closed():
return current_page
return context.new_page()


def register_session_commands(cli):
"""Register session commands on the main CLI group."""

Expand Down Expand Up @@ -368,7 +409,12 @@ def register_session_commands(cli):
"Requires: pip install 'notebooklm[cookies]'"
),
)
def login(storage, browser, browser_cookies):
@click.option(
"--fresh",
is_flag=True,
help="Delete saved browser session before login so you can choose a different account.",
)
def login(storage, browser, browser_cookies, fresh):
"""Log in to NotebookLM via browser.

Opens a browser window for Google login. After logging in,
Expand Down Expand Up @@ -399,6 +445,20 @@ def login(storage, browser, browser_cookies):

storage_path = Path(storage) if storage else get_storage_path()
browser_profile = get_browser_profile_dir()

if fresh:
try:
removed = _clear_auth_files(storage_path, browser_profile)
except OSError as e:
console.print(
"[red]Could not clear the saved browser session.[/red]\n"
"Close any running NotebookLM/Chromium windows and try again.\n"
f"Details: {e}"
)
raise SystemExit(1) from e
if removed:
console.print("[yellow]Cleared saved browser session for a fresh login.[/yellow]")

if sys.platform == "win32":
# On Windows < Python 3.13, mode= is ignored by mkdir(). On
# Python 3.13+, mode= applies Windows ACLs that can be overly
Expand Down Expand Up @@ -463,12 +523,31 @@ def login(storage, browser, browser_cookies):
break
except PlaywrightError as exc:
error_str = str(exc)
is_target_closed = (
"target page, context or browser has been closed" in error_str.lower()
)
is_retryable = any(
code in error_str for code in RETRYABLE_CONNECTION_ERRORS
)

# Check if we should retry
if is_retryable and attempt < LOGIN_MAX_RETRIES:
if is_target_closed and attempt < LOGIN_MAX_RETRIES:
page = _get_recovered_page(context, page)
backoff_seconds = attempt
console.print(
f"[yellow]Browser page was replaced during login "
f"(attempt {attempt}/{LOGIN_MAX_RETRIES}). "
f"Retrying in {backoff_seconds}s...[/yellow]"
)
time.sleep(backoff_seconds)
elif is_target_closed:
logger.error("Login page kept closing during account switch")
console.print(
"[red]The login page kept closing while switching accounts.[/red]\n"
"Retry with 'notebooklm login --fresh' to start from a clean browser session."
)
raise SystemExit(1) from None
elif is_retryable and attempt < LOGIN_MAX_RETRIES:
# Retryable error with attempts remaining: retry
backoff_seconds = attempt # Linear backoff: 1s, 2s
logger.debug(
Expand Down Expand Up @@ -948,3 +1027,26 @@ def format_cookie_name(name: str) -> str:
console.print(
"\n[yellow]Cookies may be expired. Run 'notebooklm login' to refresh.[/yellow]"
)

@auth_group.command("logout")
def auth_logout():
"""Clear saved authentication state for the active profile."""
storage_path = get_storage_path()
browser_profile = get_browser_profile_dir()

try:
removed = _clear_auth_files(storage_path, browser_profile)
except OSError as e:
console.print(
"[red]Could not remove saved auth files.[/red]\n"
"Close any running browser windows for this profile and try again.\n"
f"Details: {e}"
)
raise SystemExit(1) from e

if removed:
console.print(
"[green]Logged out. Removed saved browser session and auth state.[/green]"
)
else:
console.print("[yellow]No saved auth state found for the active profile.[/yellow]")
127 changes: 126 additions & 1 deletion tests/unit/cli/test_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import pytest
from click.testing import CliRunner

import notebooklm.cli.session as session_module
from notebooklm.notebooklm_cli import cli
from notebooklm.types import Notebook

Expand Down Expand Up @@ -234,6 +235,39 @@ def goto_side_effect(url, **kwargs):
assert result.exit_code == 0
assert "Authentication saved" in result.output

def test_login_fresh_clears_saved_auth_state(self, runner, tmp_path):
"""Test login --fresh removes both storage and browser profile before launch."""
storage_file = tmp_path / "storage.json"
storage_file.write_text("{}")
browser_profile = tmp_path / "browser_profile"
browser_profile.mkdir()
(browser_profile / "Cookies").write_text("cookie-db")

with (
patch("notebooklm.cli.session._ensure_chromium_installed"),
patch("playwright.sync_api.sync_playwright") as mock_pw,
patch("notebooklm.cli.session.get_storage_path", return_value=storage_file),
patch("notebooklm.cli.session.get_browser_profile_dir", return_value=browser_profile),
patch("notebooklm.cli.session._sync_server_language_to_config"),
patch("builtins.input", return_value=""),
):
mock_context = MagicMock()
mock_page = MagicMock()
mock_page.url = "https://notebooklm.google.com/"
mock_context.pages = [mock_page]
mock_context.storage_state.side_effect = lambda path: Path(path).write_text("{}")
mock_launch = (
mock_pw.return_value.__enter__.return_value.chromium.launch_persistent_context
)
mock_launch.return_value = mock_context

result = runner.invoke(cli, ["login", "--fresh"])

assert result.exit_code == 0
assert storage_file.exists()
assert "Cleared saved browser session" in result.output
assert mock_launch.call_args.kwargs["user_data_dir"] == str(browser_profile)

def test_login_reraises_non_navigation_playwright_errors(
self, runner, mock_login_browser_with_storage
):
Expand Down Expand Up @@ -268,6 +302,56 @@ def test_login_uses_commit_wait_strategy(self, runner, mock_login_browser_with_s
assert goto_calls[1].kwargs.get("wait_until") == "commit"
assert goto_calls[2].kwargs.get("wait_until") == "commit"

def test_login_recovers_from_target_closed_error(self, runner, tmp_path):
"""Test login retries with a fresh page when account switching closes the page."""
from playwright.sync_api import Error as PlaywrightError

storage_file = tmp_path / "storage.json"
browser_profile = tmp_path / "browser_profile"

with (
patch("notebooklm.cli.session._ensure_chromium_installed"),
patch("playwright.sync_api.sync_playwright") as mock_pw,
patch("notebooklm.cli.session.get_storage_path", return_value=storage_file),
patch("notebooklm.cli.session.get_browser_profile_dir", return_value=browser_profile),
patch("notebooklm.cli.session._sync_server_language_to_config"),
patch("builtins.input", return_value=""),
patch("notebooklm.cli.session.time.sleep"),
):
mock_context = MagicMock()
first_page = MagicMock()
first_page.url = "https://notebooklm.google.com/"
replacement_page = MagicMock()
replacement_page.url = "https://notebooklm.google.com/"
replacement_page.is_closed.return_value = False
mock_context.pages = [replacement_page]
mock_context.storage_state.side_effect = lambda path: Path(path).write_text("{}")

call_count = 0

def goto_side_effect(url, **kwargs):
nonlocal call_count
call_count += 1
if call_count == 1:
first_page.is_closed.return_value = True
raise PlaywrightError(
"Page.goto: Target page, context or browser has been closed"
)

first_page.goto.side_effect = goto_side_effect
mock_context.new_page.return_value = replacement_page
mock_context.pages = [first_page]
mock_launch = (
mock_pw.return_value.__enter__.return_value.chromium.launch_persistent_context
)
mock_launch.return_value = mock_context

result = runner.invoke(cli, ["login"])

assert result.exit_code == 0
assert replacement_page.goto.called
assert "Authentication saved" in result.output

def test_login_retries_on_connection_closed_error(
self, runner, mock_login_browser_with_storage
):
Expand Down Expand Up @@ -559,7 +643,6 @@ def test_status_json_output_with_context(self, runner, mock_context_file):
result = runner.invoke(cli, ["status", "--json"])

assert result.exit_code == 0
# Should be valid JSON
output_data = json.loads(result.output)
assert output_data["has_context"] is True
assert output_data["notebook"]["id"] == "nb_json_test"
Expand Down Expand Up @@ -587,6 +670,48 @@ def test_status_handles_corrupted_context_file(self, runner, mock_context_file):
assert result.exit_code == 0


# =============================================================================
# AUTH COMMAND TESTS
# =============================================================================


class TestAuthCommands:
def test_auth_logout_removes_profile_auth_files(self, runner, tmp_path):
"""Test auth logout deletes saved storage state and browser profile."""
storage_file = tmp_path / "storage_state.json"
storage_file.write_text("{}")
browser_profile = tmp_path / "browser_profile"
browser_profile.mkdir()
(browser_profile / "Cookies").write_text("cookie-db")

with (
patch("notebooklm.cli.session.get_storage_path", return_value=storage_file),
patch("notebooklm.cli.session.get_browser_profile_dir", return_value=browser_profile),
patch.object(
session_module, "_clear_auth_files", return_value=[storage_file, browser_profile]
),
):
result = runner.invoke(cli, ["auth", "logout"])

assert result.exit_code == 0
assert "Logged out" in result.output

def test_auth_logout_is_noop_when_nothing_saved(self, runner, tmp_path):
"""Test auth logout reports cleanly when there is nothing to remove."""
storage_file = tmp_path / "missing-storage.json"
browser_profile = tmp_path / "missing-browser-profile"

with (
patch("notebooklm.cli.session.get_storage_path", return_value=storage_file),
patch("notebooklm.cli.session.get_browser_profile_dir", return_value=browser_profile),
patch.object(session_module, "_clear_auth_files", return_value=[]),
):
result = runner.invoke(cli, ["auth", "logout"])

assert result.exit_code == 0
assert "No saved auth state found" in result.output


# =============================================================================
# CLEAR COMMAND TESTS
# =============================================================================
Expand Down
Loading