Repository navigation
fix(manual): say that Chrome capture starts with a fresh profile - #157
namanarora00 wants to merge 1 commit into
Conversation
Manual capture printed "using real chrome (profile copy)" and the class docstring promised the user's existing profile, but _start_with_real_chrome has always launched Chrome on an empty temporary profile: the user's profile directory is only checked to see that Chrome is installed. Users expecting their logins found themselves signed out with no explanation. Make the message, docstrings and manual-mode docs say the browser starts with a fresh profile and that sign-in happens during the capture. Copying the real profile is left out on purpose: it is large, locked while Chrome runs, and would duplicate the user's cookies and saved credentials into a temp dir.
9f0eead to
5de50ab
Compare
| user_profile = tmp_path / "Chrome" | ||
| user_profile.mkdir() |
There was a problem hiding this comment.
Profile copying goes undetected
The new test cannot detect copying the user's profile because user_profile is empty. Copying its contents into the separate launch directory still passes every assertion; an in-memory mutation confirmed this. This is a non-blocking coverage gap: a future change could copy existing logins while the test passes and the message still promises a fresh profile.
Add a synthetic cookie or settings file, assert that the source remains unchanged, and inspect user_data_dir inside the mocked launch call to check that the file was not copied. Check there because the temporary directory is deleted before the method returns.
Artifacts
- The executed shell runner records the original pytest result and both runtime comparisons, making the validation reproducible.
- The executed Python source injects copying only in memory and observes launch-directory contents, demonstrating the coverage gap without editing tracked files.
- The original targeted pytest test was executed without mutation and passed, establishing the baseline.
- The unchanged method was executed with empty and populated source profiles, showing an empty launch destination in both cases.
- The mutated method passed the original test and copied the populated fixture into the observed launch destination, confirming the missing destination-content check.
Ran code and verified through TREX
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_manual_browser_profile.py
Line: 17-18
Comment:
**Profile copying goes undetected**
The new test cannot detect copying the user's profile because `user_profile` is empty. Copying its contents into the separate launch directory still passes every assertion; an in-memory mutation confirmed this. This is a non-blocking coverage gap: a future change could copy existing logins while the test passes and the message still promises a fresh profile.
Add a synthetic cookie or settings file, assert that the source remains unchanged, and inspect `user_data_dir` inside the mocked launch call to check that the file was not copied. Check there because the temporary directory is deleted before the method returns.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Why
Manual capture in Chrome prints
using real chrome (profile copy), andManualBrowser's docstring says it uses "your actual Chrome browser with existing profile". It doesn't. Since_start_with_real_chromewas added (ae0206f), it has launched Chrome withlaunch_persistent_contexton an emptytempfile.mkdtemp()directory. The user's profile directory is only checked to see that Chrome is installed, and nothing is copied from it. Users who expect their logins and cookies find themselves signed out, with a message that suggests the opposite.What
tests/test_manual_browser_profile.pypins the behavior: the browser runs on the Chrome channel,user_data_diris not the user's profile, nothing is written into the user's profile, and the message says "fresh profile" rather than "profile copy".I left out actually copying the profile on purpose. A Chrome profile is often gigabytes, it is locked while Chrome runs, and copying it would duplicate the user's cookies and saved credentials into a temp directory. Cookie decryption on the copy also depends on the OS. If you want that behavior, I'd rather discuss it separately.
Verification
mainon theprofile copymessage and passes with the fix.pytest: 1232 passed, 10 skipped (1231 onmain, plus 1 new).ruff check src testsandmypy srcare clean.Each of the three PRs I'm opening (this one, the list/show cost fix and the OpenCode cost fix) adds lines in the same CHANGELOG section. Whichever merges second will need a small conflict resolution there.
Summary by cubic
Fixes the misleading message and docs for manual Chrome capture, which said it used a "profile copy" of your Chrome but has always launched Chrome with a fresh, empty temporary profile. The message, docstrings, and manual-mode docs now say the capture starts signed out and to sign in during the capture.
Written for commit 5de50ab. Summary will update on new commits.
No blocking issue was established; the remaining regression-test coverage gap does not prevent merging.
Findings
Fix with agent prompt
Summary
This PR corrects manual capture instructions without changing how Chrome launches. The console message and documentation now explain that capture starts without existing logins, and the new test checks the Chrome channel, separate profile directory, and corrected message.
One non-blocking test gap remains in
tests/test_manual_browser_profile.py: the empty source profile cannot establish that existing profile files are never copied. The test still passed when profile copying was introduced in memory. This concern does not prevent merging.Intentional scope: namanarora00 explicitly deferred actually copying the user's Chrome profile because profiles can be large and locked, copying would duplicate cookies and saved credentials into temporary storage, and cookie decryption depends on the operating system.
Reviews (1) · Last reviewed commit: "fix(manual): say that Chrome capture sta..." · Reviewed by Greptile