fix: migration overwrites stale profile cookies with fresh login data#247
Conversation
📝 WalkthroughWalkthroughMigration now compares source and destination modification times when copying legacy files into ChangesMigration timestamp precedence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the migration logic in src/notebooklm/migration.py to overwrite destination files if the source file is newer, ensuring that fresh data written to legacy paths is correctly migrated. I have no feedback to provide.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/notebooklm/migration.py (1)
82-87: Please add explicit tests for both mtime branches.The new condition introduces two critical paths (
dst >= srcskip,src > dstoverwrite) that are not directly asserted in current migration tests. Add focused cases to prevent regressions in this auth-critical flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notebooklm/migration.py` around lines 82 - 87, Add two focused unit tests that exercise the file-copy mtime branches in migration.py's loop over legacy_files: (1) create a legacy source file and a destination file where dst.stat().st_mtime >= src.stat().st_mtime, run the migration routine that iterates legacy_files, and assert the destination file was not overwritten and the logger emitted the "Skipping %s (profile copy is same age or newer)" message; (2) create a legacy source file and a destination file where src.stat().st_mtime > dst.stat().st_mtime, run the same migration routine, and assert the destination was overwritten (content changed) by the copy. Use tmp_path (or tempfile) and os.utime to set mtimes deterministically, locate the files used by the migration via the same path logic that computes dst = default_dir / src.name, and verify behavior for the legacy_files -> dst copy branch governed by the dst.exists() and mtime comparison (dst.stat().st_mtime >= src.stat().st_mtime).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/notebooklm/migration.py`:
- Around line 82-87: Add two focused unit tests that exercise the file-copy
mtime branches in migration.py's loop over legacy_files: (1) create a legacy
source file and a destination file where dst.stat().st_mtime >=
src.stat().st_mtime, run the migration routine that iterates legacy_files, and
assert the destination file was not overwritten and the logger emitted the
"Skipping %s (profile copy is same age or newer)" message; (2) create a legacy
source file and a destination file where src.stat().st_mtime >
dst.stat().st_mtime, run the same migration routine, and assert the destination
was overwritten (content changed) by the copy. Use tmp_path (or tempfile) and
os.utime to set mtimes deterministically, locate the files used by the migration
via the same path logic that computes dst = default_dir / src.name, and verify
behavior for the legacy_files -> dst copy branch governed by the dst.exists()
and mtime comparison (dst.stat().st_mtime >= src.stat().st_mtime).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfdd0822-04f3-48a2-bc81-3cb3288a81f2
📒 Files selected for processing (1)
src/notebooklm/migration.py
teng-lin
left a comment
teng-lin
left a comment
There was a problem hiding this comment.
Thanks for this fix, @LittleBitPlanet! The root cause analysis in the PR description is excellent — the comment/code mismatch where "skip if destination already exists and is newer" was never actually implemented is a great catch.
The fix itself is correct and minimal: >= with shutil.copy2's mtime preservation gives you idempotency for free. All 17 CI checks pass and the code reads cleanly.
Multi-model review summary
I ran this through several review passes (including error-handling, test-coverage, and type-design analysis). Here's the consensus:
✅ Strengths
- Fix implements what the original comment always intended — clean, minimal 4-line diff
>=comparison correctly handles idempotency viashutil.copy2's mtime preservation- Excellent PR description with clear root cause analysis
📝 Recommendation: add mtime tests before merge
The PR changes a branch condition but doesn't add a test that would fail if the change were reverted. Two focused tests using os.utime would lock in the fix:
def test_overwrites_when_source_is_newer(self, tmp_path):
"""Source file newer than profile copy triggers overwrite (the bug fix)."""
default_dir = tmp_path / "profiles" / "default"
default_dir.mkdir(parents=True)
dst = default_dir / "storage_state.json"
dst.write_text('{"cookies": ["old"]}')
os.utime(dst, (1_000_000, 1_000_000))
src = tmp_path / "storage_state.json"
src.write_text('{"cookies": ["fresh"]}')
os.utime(src, (2_000_000, 2_000_000))
with patch.dict(os.environ, {"NOTEBOOKLM_HOME": str(tmp_path)}, clear=True):
migrate_to_profiles()
assert json.loads(dst.read_text()) == {"cookies": ["fresh"]}
def test_skips_when_destination_is_newer(self, tmp_path):
"""Profile copy newer than legacy source is preserved."""
default_dir = tmp_path / "profiles" / "default"
default_dir.mkdir(parents=True)
src = tmp_path / "storage_state.json"
src.write_text('{"cookies": ["stale"]}')
os.utime(src, (1_000_000, 1_000_000))
dst = default_dir / "storage_state.json"
dst.write_text('{"cookies": ["current"]}')
os.utime(dst, (2_000_000, 2_000_000))
with patch.dict(os.environ, {"NOTEBOOKLM_HOME": str(tmp_path)}, clear=True):
migrate_to_profiles()
assert json.loads(dst.read_text()) == {"cookies": ["current"]}💡 Minor observations (not blocking)
-
Directory migration inconsistency — Lines 94-101 still use the old
if dst.exists(): skippattern forbrowser_profile/. If browser profile data can be regenerated at the legacy root after a previous migration, the same stale-overwrite issue could apply. Might be worth a follow-up issue. -
FAT32 mtime granularity — On FAT32/exFAT, mtime has 2-second precision, so a login within the same 2-second window as a previous copy could produce a false tie. Extremely unlikely in practice but a brief code comment would be nice.
Overall this is a solid, well-motivated fix. Just the two tests to add and it's good to go. 🎉
🤖 Generated with Claude Code
Follow-up: deeper investigationAfter a more thorough investigation of the code flow, I want to flag some concerns for @teng-lin's consideration. The described scenario may be unreachableThe PR describes a "recurring auth failure cycle" where
There is no code path in this codebase that recreates The only ways I can see this triggering are:
The fix itself is correct but the narrative may be overstatedThe code change is harmless and technically sound — implementing the mtime check that the original comment always described. The However, the "recurring auth failure cycle" framing suggests a critical production bug, when the actual impact appears limited to scenarios involving external file creation outside this tool's control. Contributor investigationThe contributor account (created 2026-01-21) has no prior activity. The fork → branch → PR was completed in ~61 seconds. The commit is co-authored with "Claude Opus 4.6 (1M context)". This appears to be an AI-generated contribution. RecommendationThe fix is correct and harmless — I'd still accept it (with the mtime tests I suggested earlier), but wanted to flag these findings for transparency. @teng-lin, you're the best judge of whether there's a scenario I'm missing. 🤖 Generated with Claude Code |
|
Plausible bug fix and the change is small. Before merge, please add a regression test that exercises the migration path: a stale profile cookie should be overwritten by fresh login data. A unit test in |
The migration from legacy flat layout to profiles/ skipped copying when
the destination file already existed, regardless of timestamps. This
caused a recurring auth failure cycle:
1. `notebooklm login` writes fresh cookies to the legacy root path
(because _legacy_fallback resolves there when the root file exists)
2. Next CLI command triggers ensure_profiles_dir() → migrate_to_profiles()
3. Migration sees profiles/default/storage_state.json already exists,
skips the copy, then deletes the fresh root file
4. The stale profile copy (from a prior migration) is now the only auth
source → auth fails
Fix: compare st_mtime before skipping. If the legacy root file is newer
than the profile copy, overwrite it. This matches the original comment
intent ("skip if destination already exists and is newer") which was
never implemented in the condition.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cover both directions of the new st_mtime check: - Stale profile copy is overwritten when legacy root is newer - Up-to-date profile copy is preserved when legacy root is older The first case is the regression target — verified to fail against the pre-fix migrate_to_profiles.
teng-lin
force-pushed
the
fix/migration-stale-overwrite
branch
from
72f8fb2 to
15a2c82
Compare
|
Took this over and added the regression tests. New commit
Branch was 131 commits behind Local pre-commit suite is green: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_migration.py (1)
140-181: ⚡ Quick winAdd an equal-mtime regression test to lock in
>=semantics.These two tests cover newer/older precedence well, but they don’t exercise the “same timestamp” branch that the migration logic now relies on for idempotency with
shutil.copy2. A small third test with equal mtimes would prevent regressions in that exact contract.🤖 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 `@tests/unit/test_migration.py` around lines 140 - 181, Add a third unit test to assert the migration's "equal mtime" branch preserves the profile copy (locking in >= semantics): create a default profile file "storage_state.json" and a legacy "storage_state.json" with identical mtimes, call migrate_to_profiles(), then assert the profile file still contains the profile data and the legacy file was removed; name the test something like test_keeps_profile_when_mtimes_equal and reference migrate_to_profiles to locate the migration logic.
🤖 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 `@tests/unit/test_migration.py`:
- Around line 140-181: Add a third unit test to assert the migration's "equal
mtime" branch preserves the profile copy (locking in >= semantics): create a
default profile file "storage_state.json" and a legacy "storage_state.json" with
identical mtimes, call migrate_to_profiles(), then assert the profile file still
contains the profile data and the legacy file was removed; name the test
something like test_keeps_profile_when_mtimes_equal and reference
migrate_to_profiles to locate the migration logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 615acd16-1fa8-4cb4-b04c-a323b901f94d
📒 Files selected for processing (2)
src/notebooklm/migration.pytests/unit/test_migration.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/notebooklm/migration.py
2 participants
Summary
profiles/default/skipped copyingstorage_state.jsonwhen the destination already existed — regardless of whether the source was newerloginwrites fresh cookies to the legacy root path, migration deletes the fresh file but keeps the stale profile copy, auth failsst_mtimebefore skipping — if the legacy root file is newer, overwrite the profile copyRoot Cause
_legacy_fallback()inpaths.pyresolvesget_storage_path()to the root~/.notebooklm/storage_state.jsonwhen it exists (for backwards compat). Sologinwrites there. Butensure_profiles_dir()runs on every CLI invocation and triggersmigrate_to_profiles()whenever legacy files exist at root. The migration copied root → profile on first run, but on subsequent runs it saw the profile copy already existed and skipped the copy — then deleted the (newer) root file anyway.The original comment even said "skip if destination already exists and is newer" but the
and is newerpart was never implemented in the condition.Test plan
ruff format,ruff check,mypypass (confirmed locally)login→ next CLI command → auth still works (fresh cookies preserved in profile)🤖 Generated with Claude Code
Summary by CodeRabbit