Run browser_cookie3 at arm's length; make usage stats opt-in; fix the non-TTY crash - #19
Conversation
Pre-announce pass, found by installing from the published curl|bash installer into a bare debian:12-slim container. LGPL. browser-cookie3 is LGPL-3.0 (confirmed against upstream's LICENSE and setup.py) and was a hard dependency imported directly into agent.py. A direct import makes the two a combined work under LGPL section 4, which imposes relinking obligations this MIT project doesn't acknowledge. Same trap we already avoid with piper-tts in saccade, where the rule is: run GPL-family code as a subprocess, never import it. It now runs in a child interpreter and returns JSON, so the LGPL code never enters this process. Moved it from `dependencies` to a `chrome-cookies` extra. Without it installed, cookie import returns None and the manual sign-in flow takes over, which was already the fallback. Asserted in a test that browser_cookie3 stays out of sys.modules both after importing agent and after calling the function. Non-TTY crash. Running `autofill` without a TTY (pipe, cron, CI, some IDE terminals) dumped a prompt_toolkit stack trace ending in a bare EOFError. cli() already converted Ctrl-C into a clean exit; EOFError now gets the same treatment with an actionable message. Verified in the container that produced the original traceback. Docs. README claimed "Requires Python 3.11+" but never mentioned git, which the installer needs and which a clean box lacks. It also doesn't need Python at all, since the installer brings uv. Removed the duplicate "Built on browser-use" line eight lines below the first one. Em-dashes: 77 removed from Python, including the onboarding banner every new user sees, plus the remaining doc files. Realigned the telemetry docstring table where the em-dash was a column separator. ruff, ty, and 128 tests green.
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef924dad37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two pre-launch fixes on top of the LGPL isolation work.
Usage stats are now off unless the user turns them on. AUTOFILL_TELEMETRY
defaults to "0" and only an explicit "1" enables sending, so an unset
variable means no events. Setup asks once (_onboard_telemetry), the prompt
default is no, and the answer is written to .env so it survives an
uninstall/reinstall. Nothing is sent before the question is answered.
Crash reports stay env-var only and are deliberately not prompted for:
Sentry stack frames can incidentally capture profile data, which is not
something to solicit a click-through yes for during onboarding. Both
signals now default to off, which is the right posture for a tool that
reads resumes and identity documents.
The em-dash removal in the previous commit left comma splices and one
garbled comment ("from a weak model: or a 429/5xx: otherwise") across
README, CONTRIBUTING, AGENTS, CLAUDE, the bug-report template, and two
user-facing warnings. Replaced with colons and semicolons that parse.
Verified: 143 tests pass, ruff clean, and the opt-in flow checked end to
end against a redirected .env (default declines, answer persists through
load_dotenv, re-answering does not duplicate the line).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
jayzuccarelli
changed the title
Moving browser-cookie3 behind an optional extra fixed the LGPL import problem but silently broke the feature: install.sh ran `uv sync --quiet` and wrote a wrapper calling `uv run autofill`, neither of which pulls an extra. Every install would have shipped without browser-cookie3 while onboarding still asked "Import Chrome logins now?" with a default of yes, so the answer would always have been "Couldn't read Chrome cookies". Both sync sites and the wrapper now pass --extra chrome-cookies. The wrapper needs it too, not just the sync: uv prunes an extra back out of the environment on a later run that omits the flag. This does not weaken the licensing fix. The LGPL concern is the direct import creating a combined work, not the package being present; invoking it as a separate program stays at arm's length either way. Found by the Codex reviewer on the PR. Verified: bash -n clean, --extra is valid on both uv sync and uv run, 143 tests pass, ruff clean, and browser_cookie3 is still absent from the parent's sys.modules before and after _import_chrome_cookies() runs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Pre-announce verification pass. I installed autofill from the published
curl | bashinstaller into a baredebian:12-slimcontainer, i.e. exactly what a Show HN reader runs.1. LGPL dependency imported into an MIT project (the one with teeth)
browser-cookie3is LGPL-3.0, confirmed against upstream and PyPI (license: lgpl). It was a hard dependency inpyproject.tomland imported directly atagent.py:967. A direct Python import makes the two a combined work under LGPL section 4, imposing relinking and replaceability obligations the MITLICENSEdoesn't acknowledge.This is the same trap already avoided in saccade, where piper-tts (GPL-3.0) is deliberately run via
create_subprocess_execand never imported. Applied the same rule here.Fix: the cookie export runs in a child interpreter and returns JSON. Moved to a
chrome-cookiesoptional extra. Without it installed, the function returnsNoneand the manual sign-in flow takes over, which was already the documented fallback.Proof the boundary holds:
Tests pin it: one runs the real subprocess against a stub module and asserts the child's output is byte-identical to
_cookiejar_to_storage_state, so the two can't drift.2. Non-TTY invocation dumped a stack trace
Running
autofillwithout a TTY (pipe, cron, CI, some IDE terminals) ended in a rawprompt_toolkittraceback and a bareEOFError.cli()already turned Ctrl-C into a clean exit;EOFErrornow gets the same treatment.Before: 12 lines of traceback. After, verified in the same container:
3. Usage stats are now opt-in
Telemetry defaulted to on. On a CLI that reads resumes and identity documents, opt-out analytics is a reliable thread-derailer, and it sat awkwardly next to Sentry, which was already correctly opt-in.
Now:
AUTOFILL_TELEMETRYdefaults to"0"and only an explicit"1"enables sending, so an unset variable means nothing is sent. Setup asks once, the prompt default is no, and the answer is written to.envso it survives an uninstall/reinstall. Nothing is collected before the user has answered.Crash reports stay env-var only and are deliberately not prompted for: Sentry stack frames can incidentally capture profile data, which is not something to solicit a click-through yes for during onboarding. Two mechanisms, one question, both default off.
Verified end to end against a redirected
.env:4. Docs vs reality
Install git, or clone the repo manually, which is a good message for a requirement that was undocumented.Built on [browser-use](...)line sitting eight lines below the first one on the front page.5. Em-dashes, and the grammar they took with them
77 removed from Python, including the onboarding banner every new user sees, plus the remaining doc files.
The first pass replaced several with commas, which left comma splices in
CONTRIBUTING.md,CLAUDE.md,AGENTS.md, the bug-report template, and two user-facing warnings, plus one outright garbled comment (# A malformed step from a weak model: or a 429/5xx: otherwise). Those are the files a Show HN visitor actually opens, so they're now colons and semicolons that parse.Verification
ruffclean, 143 tests pass. Installer verified end to end on a clean container: uv bootstrap, clone at pinned tag v0.2.2,--help, bad-URL handling (fails cleanly), and the interactive path rendering correctly under a real TTY.Not fixed, needs your call
requires-pythonis>=3.11and the classifiers claim only 3.11/3.12. Users get an untested interpreter by default. Consider testing 3.13/3.14 in CI or capping the range.posthog-setup-report.mdand.autofill_install_idare deleted from the tree but recoverable from commits528c0a8/7ad2f06, exposing a PostHog project ID and six dashboard URLs. Also 51JAY-NNtracker refs. All need onegit filter-repopass, which would orphan the existing release tags, so it is deliberately not done here.