Skip to content

docs: document the long-running (deep-research) cassette re-record recipe#1567

Merged
teng-lin merged 2 commits into
mainfrom
docs/long-running-cassette-rerecord
Jun 12, 2026
Merged

docs: document the long-running (deep-research) cassette re-record recipe#1567
teng-lin merged 2 commits into
mainfrom
docs/long-running-cassette-rerecord

Conversation

@teng-lin

@teng-lin teng-lin commented Jun 12, 2026

Copy link
Copy Markdown
Owner

Captures the non-obvious snags from the v0.8 full-lifecycle Deep Research re-record (PR #1566) so the next maintainer who re-records a long-running cassette doesn't rediscover them.

Adds a "Long-running recordings (deep research, multi-minute polling)" subsection to docs/development.md:

  • httpx.PoolTimeout after ~15–20 min idle polling → record-mode timeout=60 + ConnectionLimits(keepalive_expiry=10.0) (and the note that async_client_factory is not a public constructor kwarg).
  • Global pytest-timeout kills the run → @pytest.mark.timeout(3600).
  • Deep Research start() task_id ≠ the poll-reported id → filtered poll returns NOT_FOUND; mirror wait_for_completion (unfiltered first poll, then pin).
  • Trim via a byte-exact YAML text slice, not yaml.safe_dump (which re-wraps long scalars and breaks Windows YAML parsing — CI caught it).
  • Cleanliness is necessary-not-sufficient: grep for live credential shapes beyond the name-anchored scrubber.

Docs-only; no code or test changes.

🤖 Generated with Claude Code

Review in cubic

Summary by CodeRabbit

  • Documentation
    • Added guidance for long-running test recordings: manage timeouts and keepalive settings to avoid pool timeouts, extend test timeouts for multi-minute recordings, ensure polling uses the reported task identifier, trim oversized in-progress poll bodies using byte-exact slicing, and verify cassette cleanliness with both automated guards and manual pattern checks for live credential artifacts.

…cipe

Capture the non-obvious snags from the v0.8 full-lifecycle Deep Research
re-record (PR #1566) under docs/development.md "Long-running recordings":
record-mode httpx tuning to dodge httpx.PoolTimeout (timeout=60 +
ConnectionLimits(keepalive_expiry=10); async_client_factory is not public),
@pytest.mark.timeout(3600), the deep start() task_id != poll-reported id
NOT_FOUND trap, byte-exact YAML trimming (yaml.safe_dump breaks Windows
parsing), and credential-grep beyond the name-anchored scrubber.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@teng-lin

Copy link
Copy Markdown
Owner Author

@claude review

@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1d183435-a915-4668-b3e3-baaae2292731

📥 Commits

Reviewing files that changed from the base of the PR and between 3b07134 and de7a6cf.

📒 Files selected for processing (1)
  • docs/development.md

📝 Walkthrough

Walkthrough

A new subsection under "VCR Testing (Recorded HTTP)" documents how to record long-running Deep Research operations, covering HTTP client timeout tuning, pytest-timeout extension, correct polling task-id usage, cassette size reduction via text slicing, and credential verification strategies.

Changes

VCR Testing Documentation for Long-Running Operations

Layer / File(s) Summary
Long-running recording documentation
docs/development.md
New subsection documents handling multi-minute Deep Research VCR recordings: httpx timeout settings for idle polling, extended pytest-timeout configuration, using poll-reported task_id for polling, byte-exact text slicing to trim poll bodies (avoiding YAML parsing), and verification steps using cassette guard and grep for live credentials.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • teng-lin/notebooklm-py#645: Related VCR deep-research polling-loop integration test and handling/assertions for polling behavior and cassette hygiene.
  • teng-lin/notebooklm-py#1568: Related update supporting VCR integration test recording/replay for ResearchAPI polling with customized timeout settings.

Poem

🐰 A guide for the patient heart,
When Deep Research plays its part,
Poll timeouts tamed, credentials scrubbed clean,
VCR cassettes recorded serene,
Long-running dreams, now documented with care! 📖

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: documenting a cassette re-record recipe for long-running Deep Research operations in the development guide.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docs/long-running-cassette-rerecord

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Claude finished @teng-lin's task in 1m 32s —— View job


Code Review

  • Read changed files
  • Review documentation content
  • Post findings

Overall: LGTM — well-written and captures genuinely non-obvious pitfalls.

The new Long-running recordings subsection fits cleanly between "Cassette recording" and "Synthetic error cassettes" and documents all five snags discovered during the v0.8 Deep Research re-record. The bold-name pattern (cause → effect → fix) is consistent with the rest of the file and makes each bullet scannable.

A few minor suggestions:

1. Missing import hint for ConnectionLimits (docs/development.md, ~line 594–595)

The httpx bullet shows the full constructor call but doesn't mention the import. A future maintainer following this recipe may not know ConnectionLimits comes from httpx:

from httpx import ConnectionLimits
NotebookLMClient(auth, timeout=60.0, limits=ConnectionLimits(keepalive_expiry=10.0))

Adding from httpx import ConnectionLimits inline (or noting the import origin) would save a small look-up.

Fix this →

2. Unquantified "cassette size cap" (docs/development.md, ~line 606)

trim redundant middle in_progress polls to stay under the cassette size cap

Where is this cap defined? A future maintainer who hits it will want to know the limit (in KB/MB) or at least where to find the enforcement (e.g., a constant in vcr_config.py or a CI check). Adding a parenthetical like "see MAX_CASSETTE_BYTES in tests/vcr_config.py" (or whatever the actual location is) would anchor this.

3. Bare PR reference may not auto-link in doc exports (docs/development.md, line 588)

PR #1566 works as a GitHub auto-link when viewed on GitHub, but in rendered documentation or docs mirrors it'll be plain text. Consider [PR #1566](https://github.com/teng-lin/notebooklm-py/pull/1566) for robustness. Minor — the current form is fine for an internal-only dev guide.


None of the above block merge; the content is accurate and the placement is correct. The SAPISIDHASH grep pattern in the last bullet is appropriately distinct from the SAPISID cookie shape the logging redacter covers, which is a good catch.

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request adds a new section to "docs/development.md" documenting guidelines and lessons learned for handling long-running recordings and multi-minute polling. The feedback suggests updating the spelling of "Re-serialising" to "Re-serializing" to maintain consistency with the rest of the codebase.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread docs/development.md Outdated

@coderabbitai coderabbitai Bot left a comment

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.

🧹 Nitpick comments (1)
docs/development.md (1)

590-597: ⚡ Quick win

Verify the ConnectionLimits example is complete.

The live recording harness also sets max_connections=100 and max_keepalive_connections=20 alongside keepalive_expiry=10.0. If those values are part of the PoolTimeout workaround, the doc example should include them; otherwise readers may copy an incomplete recipe.

♻️ Possible doc update
- `NotebookLMClient(auth, timeout=60.0, limits=ConnectionLimits(keepalive_expiry=10.0))`.
+ `NotebookLMClient(
+     auth,
+     timeout=60.0,
+     limits=ConnectionLimits(
+         max_connections=100,
+         max_keepalive_connections=20,
+         keepalive_expiry=10.0,
+     ),
+ )`.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/development.md` around lines 590 - 597, The ConnectionLimits example is
incomplete: in the live recording harness the PoolTimeout workaround also sets
max_connections and max_keepalive_connections, so update the example for
NotebookLMClient to pass ConnectionLimits(keepalive_expiry=10.0,
max_connections=100, max_keepalive_connections=20) and keep the generous timeout
(timeout=60.0); ensure this change is applied where NotebookLMClient(...) is
constructed and preserve the note that async_client_factory is not a public
constructor kwarg while using the public timeout= / limits= parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@docs/development.md`:
- Around line 590-597: The ConnectionLimits example is incomplete: in the live
recording harness the PoolTimeout workaround also sets max_connections and
max_keepalive_connections, so update the example for NotebookLMClient to pass
ConnectionLimits(keepalive_expiry=10.0, max_connections=100,
max_keepalive_connections=20) and keep the generous timeout (timeout=60.0);
ensure this change is applied where NotebookLMClient(...) is constructed and
preserve the note that async_client_factory is not a public constructor kwarg
while using the public timeout= / limits= parameters.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 15c1178b-43a9-4795-ae78-a5c6569a1a56

📥 Commits

Reviewing files that changed from the base of the PR and between 8bcbef9 and 3b07134.

📒 Files selected for processing (1)
  • docs/development.md

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@teng-lin teng-lin merged commit 822b3a2 into main Jun 12, 2026
3 of 4 checks passed
@teng-lin teng-lin deleted the docs/long-running-cassette-rerecord branch June 12, 2026 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants