Skip to content

feat(chat): add --timeout option to ask command #260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Flosters wants to merge 1 commit into
base: main
Choose a base branch
from
+9 −1
Open

feat(chat): add --timeout option to ask command #260

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion src/notebooklm/cli/chat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,13 @@ def register_chat_commands(cli):
)
@click.option("--save-as-note", is_flag=True, help="Save response as a note")
@click.option("--note-title", default=None, help="Note title (use with --save-as-note)")
@click.option(
"--timeout",
default=120,
type=int,
show_default=True,
help="Request timeout in seconds. Increase for long prompts (default: 120).",
)
Comment on lines +105 to +111
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Apr 9, 2026

Choose a reason for hiding this comment

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

medium

The help text contains a redundant default value since show_default=True is enabled, which will cause Click to append [default: 120] automatically. Additionally, the timeout should be validated to ensure it is a positive value to avoid potential issues with the underlying network client.

Suggested change
@click.option(
"--timeout",
default=120,
type=int,
show_default=True,
help="Request timeout in seconds. Increase for long prompts (default: 120).",
)
@click.option(
"--timeout",
default=120,
type=click.IntRange(min=1),
show_default=True,
help="Request timeout in seconds. Increase for long prompts.",
)

Comment on lines +105 to +111
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 9, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

In current httpx versions, are timeout values of 0 or negative numbers valid for connect/read/write/pool timeouts?

💡 Result:

  • httpx.Timeout itself doesn’t validate ranges for connect/read/write/pool (it just stores whatever you pass). So 0 or negative floats can be constructed without error. [1]

  • But when actually used for I/O, negative timeout values are not valid:

    • On the sync path, HTTPCore uses Python sockets, and socket.settimeout(value) requires a nonnegative real number (or None); negative values are invalid. [2]
    • On the Trio async backend, Trio’s timeout context managers raise ValueError if seconds is negative. [3]

  • 0 is allowed but means “don’t wait” (e.g., Python sockets treat 0 as non-blocking mode, i.e. effectively an immediate timeout/no waiting). [2]

  • To disable timeouts, use None (per-timeout, or Timeout(None, ...)). [1]

Answer: 0 is technically allowed (immediate / non-blocking semantics), but negative numbers are not valid in practice and will error in common backends; use None to disable instead. [1][2][3]

Sources:
[1] HTTPX Timeout implementation (httpx/_config.py, master)
[2] Python socket.settimeout() docs (nonnegative real, 0 => non-blocking)
[3] Trio move_on_after() docs (negative seconds => ValueError)

Validate --timeout as strictly positive at parse time.

type=int allows 0 and negative values. Negative timeouts will fail at runtime (Python sockets and Trio backends reject non-negative values), and 0 creates non-blocking semantics without waiting. Reject these early with a CLI validation error for clearer UX.

Proposed fix 
     `@click.option`(
         "--timeout",
         default=120,
-        type=int,
+        type=click.IntRange(min=1),
         show_default=True,
-        help="Request timeout in seconds. Increase for long prompts (default: 120).",
+        help="Request timeout in seconds. Increase for long prompts.",
     )
📝 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
@click.option(
"--timeout",
default=120,
type=int,
show_default=True,
help="Request timeout in seconds. Increase for long prompts (default: 120).",
)
`@click.option`(
"--timeout",
default=120,
type=click.IntRange(min=1),
show_default=True,
help="Request timeout in seconds. Increase for long prompts.",
)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/notebooklm/cli/chat.py` around lines 105 - 111, The --timeout click
option currently uses type=int which permits 0 and negative values; change its
validation to require strictly positive integers by replacing type=int with
click.IntRange(min=1) (or add a callback that raises click.BadParameter if
timeout < 1) in the `@click.option` declaration for "--timeout" in chat.py so the
CLI rejects non-positive values at parse time and returns a clear validation
error.

@with_client
def ask_cmd(
ctx,
Expand All @@ -112,6 +119,7 @@ def ask_cmd(
json_output,
save_as_note,
note_title,
timeout,
client_auth,
):
"""Ask a notebook a question.
Expand All @@ -131,7 +139,7 @@ def ask_cmd(
nb_id = require_notebook(notebook_id)

async def _run():
async with NotebookLMClient(client_auth) as client:
async with NotebookLMClient(client_auth, timeout=float(timeout)) as client:
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Apr 9, 2026

Choose a reason for hiding this comment

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

medium

Following the general rules for network clients, it is recommended to use granular timeouts. A single float value sets the same timeout for connection and reading. Using a shorter connect timeout (e.g., 10s) while allowing a longer read timeout improves resilience by failing fast on connection issues while still accommodating slow responses. Note that the NotebookLMClient type hints should also be updated to support this.

Suggested change
async with NotebookLMClient(client_auth, timeout=float(timeout)) as client:
async with NotebookLMClient(client_auth, timeout=(10.0, float(timeout))) as client:
References
  1. For network clients, use granular timeouts with a shorter connect timeout and longer read/write timeouts to improve resilience by detecting connection issues quickly while accommodating slow responses.

nb_id_resolved = await resolve_notebook_id(client, nb_id)
effective_conv_id = _determine_conversation_id(
explicit_conversation_id=conversation_id,
Expand Down
Loading