Skip to content
Merged
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
169 changes: 154 additions & 15 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 Down Expand Up @@ -59,6 +60,17 @@
# Retryable Playwright connection errors
RETRYABLE_CONNECTION_ERRORS = ("ERR_CONNECTION_CLOSED", "ERR_CONNECTION_RESET")
LOGIN_MAX_RETRIES = 3
# Playwright TargetClosedError substring — verified against Playwright >= 1.40.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[Low] Playwright's TargetClosedError class was introduced in version 1.41 (not 1.40). Consider updating the version reference or replacing it with a source-of-truth reference:

# Playwright TargetClosedError substring — matches the default message from
# Playwright's TargetClosedError class. If a future version changes this
# message, the error will propagate unhandled (safe fallback).

Found by: Claude Comment Analyzer

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 4ac30ee: updated comment to reference v1.41 and removed the specific version claim in favor of a source-of-truth description.

# If a future Playwright version changes this message, the error will propagate
# unhandled (safe fallback) rather than silently ignored.
TARGET_CLOSED_ERROR = "Target page, context or browser has been closed"
BROWSER_CLOSED_HELP = (
"[red]The browser window was closed during login.[/red]\n"
"This can happen when switching Google accounts in a persistent browser session.\n\n"
"Try:\n"
" 1. Run: notebooklm login --fresh\n"
" 2. Or run: notebooklm auth logout && notebooklm login"
)
CONNECTION_ERROR_HELP = (
"[red]Failed to connect to NotebookLM after multiple retries.[/red]\n"
"This may be caused by:\n"
Expand Down Expand Up @@ -198,9 +210,7 @@ def _login_with_browser_cookies(storage_path: Path, browser_name: str) -> None:
storage_path.chmod(0o600)
except OSError as e:
logger.error("Failed to save authentication to %s: %s", storage_path, e)
console.print(
f"[red]Failed to save authentication to {storage_path}.[/red]\n" f"Details: {e}"
)
console.print(f"[red]Failed to save authentication to {storage_path}.[/red]\nDetails: {e}")
raise SystemExit(1) from None

console.print(f"\n[green]Authentication saved to:[/green] {storage_path}")
Expand Down Expand Up @@ -340,6 +350,23 @@ def _ensure_chromium_installed() -> None:
)


def _recover_page(context, console):
"""Get a fresh page from a persistent browser context.

Used when the current page reference is stale (TargetClosedError).
A new page in a persistent context inherits all cookies and storage.

Returns a new Page, or raises SystemExit if the context/browser is dead.
"""
from playwright.sync_api import Error as PlaywrightError

try:
return context.new_page()
except PlaywrightError as exc:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[High] PlaywrightError is the base class for all Playwright errors. If context.new_page() fails for a reason other than a dead context (resource exhaustion, browser crash, internal Playwright bug), the user sees "browser window was closed during login" which is misleading.

Suggestion — narrow the catch to only show BROWSER_CLOSED_HELP when it's actually a target-closed error:

def _recover_page(context, console):
    from playwright.sync_api import Error as PlaywrightError

    try:
        return context.new_page()
    except PlaywrightError as exc:
        error_str = str(exc)
        if TARGET_CLOSED_ERROR in error_str:
            logger.error("Browser context is dead, cannot recover page: %s", error_str)
            console.print(BROWSER_CLOSED_HELP)
            raise SystemExit(1) from exc
        # Not a TargetClosedError — don't mask the real problem
        logger.error("Failed to create new page for recovery: %s", error_str)
        raise

This also adds logger.error() which is currently missing (every other terminal error path in this file logs before exiting).

Found by: Claude Silent Failure Hunter, Codex

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 4ac30ee: narrowed catch to only show BROWSER_CLOSED_HELP when TARGET_CLOSED_ERROR is in the error string. Other PlaywrightError types now re-raise. Added logger.error() for both paths.

console.print(BROWSER_CLOSED_HELP)
raise SystemExit(1) from exc


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

Expand Down Expand Up @@ -368,7 +395,13 @@ def register_session_commands(cli):
"Requires: pip install 'notebooklm[cookies]'"
),
)
def login(storage, browser, browser_cookies):
@click.option(
"--fresh",
is_flag=True,
default=False,
help="Start with a clean browser session (deletes cached browser profile). Use to switch Google accounts.",
)
def login(storage, browser, browser_cookies, fresh):
"""Log in to NotebookLM via browser.

Opens a browser window for Google login. After logging in,
Expand All @@ -393,12 +426,30 @@ def login(storage, browser, browser_cookies):

# rookiepy fast-path: skip Playwright entirely
if browser_cookies is not None:
if fresh:
console.print(
"[yellow]Warning: --fresh has no effect with --browser-cookies "
"(no browser profile is used).[/yellow]"
)
resolved_storage = Path(storage) if storage else get_storage_path()
_login_with_browser_cookies(resolved_storage, browser_cookies)
return

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

if fresh and browser_profile.exists():
try:
shutil.rmtree(browser_profile)
console.print("[yellow]Cleared cached browser session (--fresh)[/yellow]")
except OSError as exc:
console.print(
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[Medium] This error path is missing logger.error(). Every other terminal error path in this file logs before exiting. The same issue applies to _recover_page() at line 366.

Suggestion:

except OSError as exc:
    logger.error("Failed to clear browser profile %s: %s", browser_profile, exc)
    console.print(...)

Found by: Claude Silent Failure Hunter

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 4ac30ee: added logger.error() to the --fresh OSError path and both paths in _recover_page.

"[red]Cannot clear browser profile — it may be in use by another process.[/red]\n"
"Close any open browser windows and try again.\n"
f"If the problem persists, manually delete: {browser_profile}"
)
raise SystemExit(1) from exc

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 @@ -454,7 +505,7 @@ def login(storage, browser, browser_cookies):
try:
context = p.chromium.launch_persistent_context(**launch_kwargs)

page = context.pages[0] if context.pages else context.new_page()
page = context.pages[0] if context.pages else _recover_page(context, console)

# Retry navigation on transient connection errors with backoff
for attempt in range(1, LOGIN_MAX_RETRIES + 1):
Expand All @@ -466,22 +517,41 @@ def login(storage, browser, browser_cookies):
is_retryable = any(
code in error_str for code in RETRYABLE_CONNECTION_ERRORS
)
is_target_closed = TARGET_CLOSED_ERROR in error_str

# Check if we should retry
if is_retryable and attempt < LOGIN_MAX_RETRIES:
# Retryable error with attempts remaining: retry
if (is_retryable or is_target_closed) and attempt < LOGIN_MAX_RETRIES:
# For TargetClosedError, get a fresh page reference
if is_target_closed:
page = _recover_page(context, console)

backoff_seconds = attempt # Linear backoff: 1s, 2s
logger.debug(
f"Retryable connection error on attempt {attempt}/{LOGIN_MAX_RETRIES}: {error_str}"
f"Retryable error on attempt {attempt}/{LOGIN_MAX_RETRIES}: {error_str}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

For Python logging, use the % formatting style instead of f-strings to enable lazy formatting, where string interpolation is deferred until the log is actually emitted. This is a best practice for performance and is also specified in the general rules for this repository.

                            logger.debug(
                                "Retryable error on attempt %d/%d: %s",
                                attempt,
                                LOGIN_MAX_RETRIES,
                                error_str,
                            )
References
  1. For Python logging, use the % formatting style instead of f-strings to enable lazy formatting, where string interpolation is deferred until the log is emitted.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in e53d686: switched to % formatting for lazy evaluation.

)
console.print(
f"[yellow]Connection interrupted "
f"(attempt {attempt}/{LOGIN_MAX_RETRIES}). "
f"Retrying in {backoff_seconds}s...[/yellow]"
if is_target_closed:
console.print(
f"[yellow]Browser page closed "
f"(attempt {attempt}/{LOGIN_MAX_RETRIES}). "
f"Retrying with fresh page...[/yellow]"
)
else:
console.print(
f"[yellow]Connection interrupted "
f"(attempt {attempt}/{LOGIN_MAX_RETRIES}). "
f"Retrying in {backoff_seconds}s...[/yellow]"
)
time.sleep(backoff_seconds)
elif is_target_closed:
# Exhausted retries on browser-closed errors
logger.error(
f"Browser closed during login after {LOGIN_MAX_RETRIES} attempts. "
f"Last error: {error_str}"
)
Comment on lines +562 to 566
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

For Python logging, use the % formatting style instead of f-strings to enable lazy formatting. This applies to all logging calls in this section.

Suggested change
logger.error(
f"Browser closed during login after {LOGIN_MAX_RETRIES} attempts. "
f"Last error: {error_str}"
)
logger.error(
"Browser closed during login after %d attempts. Last error: %s",
LOGIN_MAX_RETRIES,
error_str,
)
References
  1. For Python logging, use the % formatting style instead of f-strings to enable lazy formatting, where string interpolation is deferred until the log is emitted.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in e53d686: switched to % formatting for lazy evaluation.

time.sleep(backoff_seconds)
console.print(BROWSER_CLOSED_HELP)
raise SystemExit(1) from exc
elif is_retryable:
# Exhausted retries on a retryable error
# Exhausted retries on network errors
logger.error(
f"Failed to connect to NotebookLM after {LOGIN_MAX_RETRIES} attempts. "
f"Last error: {error_str}"
Expand All @@ -508,7 +578,20 @@ def login(storage, browser, browser_cookies):
try:
page.goto(url, wait_until="commit")
except PlaywrightError as exc:
if "Navigation interrupted" not in str(exc):
error_str = str(exc)
if TARGET_CLOSED_ERROR in error_str:
# Page was destroyed (e.g. user switched accounts) -- get fresh page
page = _recover_page(context, console)
try:
page.goto(url, wait_until="commit")
except PlaywrightError as inner_exc:
if TARGET_CLOSED_ERROR in str(inner_exc):
# Recovered page also dead -- context/browser is gone
console.print(BROWSER_CLOSED_HELP)
raise SystemExit(1) from inner_exc
elif "Navigation interrupted" not in str(inner_exc):
raise
elif "Navigation interrupted" not in error_str:
raise

current_url = page.url
Expand Down Expand Up @@ -745,6 +828,62 @@ def auth_group():
"""Authentication management commands."""
pass

@auth_group.command("logout")
def auth_logout():
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[Medium] login blocks when NOTEBOOKLM_AUTH_JSON is set (line 416), but auth logout has no equivalent check. After auth logout, the env var auth remains active — the user sees "Logged out" but the very next CLI command still works with the env var credentials.

Suggestion — mirror login's guard, or at minimum warn:

if os.environ.get("NOTEBOOKLM_AUTH_JSON"):
    console.print(
        "[yellow]Note: NOTEBOOKLM_AUTH_JSON is set — env-based auth will "
        "remain active after logout. Unset it to fully log out.[/yellow]"
    )

Found by: Codex (gpt-5.4)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 4ac30ee: added NOTEBOOKLM_AUTH_JSON warning at the top of auth logout, mirroring login's guard pattern.

"""Log out by clearing saved authentication.

Removes both the saved cookie file (storage_state.json) and the
cached browser profile. After logout, run 'notebooklm login' to
authenticate with a different Google account.

\b
Examples:
notebooklm auth logout # Clear auth for active profile
notebooklm -p work auth logout # Clear auth for 'work' profile
"""
storage_path = get_storage_path()
browser_profile = get_browser_profile_dir()

removed_any = False

# Remove storage_state.json
if storage_path.exists():
try:
storage_path.unlink()
removed_any = True
except OSError:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

[High] Three concerns with the OSError handling in auth logout:

  1. This except OSError: doesn't bind the exception — the error details are truly discarded
  2. from None at line 864 explicitly suppresses the exception chain
  3. No logger.error() call — failure is invisible in logs

The user always sees "may be in use by another process" regardless of actual cause (could be PermissionError, ReadOnlyFilesystemError, or even FileNotFoundError from a TOCTOU race).

Suggestion:

except OSError as exc:
    logger.error("Failed to remove auth file %s: %s", storage_path, exc)
    console.print(
        f"[red]Cannot remove auth file: {exc}[/red]\n"
        "Close any running notebooklm commands and try again.\n"
        f"If the problem persists, manually delete: {storage_path}"
    )
    raise SystemExit(1) from exc

Same pattern applies to the shutil.rmtree handler at line 871.

Found by: Claude Silent Failure Hunter

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 4ac30ee: bound OSError as exc, switched from from None to from exc for proper chaining, added logger.error(), and now show the actual error in the user message (e.g. Cannot remove auth file: {exc}).

console.print(
"[red]Cannot remove auth file — it may be in use by another process.[/red]\n"
"Close any running notebooklm commands and try again.\n"
f"If the problem persists, manually delete: {storage_path}"
)
raise SystemExit(1) from None

# Remove browser profile directory
if browser_profile.exists():
try:
shutil.rmtree(browser_profile)
removed_any = True
except OSError:
partial = (
"[yellow]Note: Auth file was removed, but browser profile "
"could not be deleted.[/yellow]\n"
if removed_any
else ""
)
console.print(
f"{partial}"
"[red]Cannot remove browser profile — it may be in use by another process.[/red]\n"
"Close any open browser windows and try again.\n"
f"If the problem persists, manually delete: {browser_profile}"
)
raise SystemExit(1) from None

if removed_any:
console.print("[green]Logged out.[/green] Run 'notebooklm login' to sign in again.")
else:
console.print("[yellow]No active session found.[/yellow] Already logged out.")
Comment on lines +847 to +910
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The auth logout command clears the authentication cookies and browser profile, but it leaves the context.json file intact. This file stores the current notebook ID and conversation ID. If a user logs out and another user logs in on the same profile, the status command will still show the previous user's notebook context until it is manually cleared. Consider also clearing the context file during logout for a cleaner session transition.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Intentionally kept separate — context.json stores notebook context (not credentials) and is owned by the clear command. Clearing it on logout would be an unexpected side effect since a user's active notebook selection is independent of which Google account is logged in. The existing notebooklm clear command handles this when needed.


@auth_group.command("check")
@click.option(
"--test", "test_fetch", is_flag=True, help="Test token fetch (makes network request)"
Expand Down
Loading
Loading