fix(tools): ship the browser recording JS helpers in the wheel - #4445
fix(tools): ship the browser recording JS helpers in the wheel#4445onatozmenn wants to merge 1 commit into
Conversation
recording.py loads six helpers from `Path(__file__).parent / "js"`, but openhands-tools declared package-data only for py.typed, Jinja templates and the subagent prompts. The JS never reached the built wheel, so collect_data_files in agent-server.spec collected an empty set and a frozen agent-server raised FileNotFoundError on browser_start_recording. A clean wheel build shows the difference: zero js/ entries before, all six after. An editable install reads the files straight from the working tree, so nothing in a source checkout surfaces this. The added test walks every non-Python file under openhands/tools that is not a contributor doc and asserts a package-data glob covers it, so a new runtime asset cannot be added without shipping it. It reproduces the same verdict the wheel build gives, including that openhands.tools.preset.subagents ships fine despite having no __init__.py. Reported by @apps3000 in OpenHands#4443, with the PyInstaller root cause. Signed-off-by: onatozmenn <onatozmen44@gmail.com>
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR fixes #4443 by declaring the six browser recording JS helpers as package data in openhands-tools/pyproject.toml. browser_use/recording.py loads these files by name from _JS_DIR = Path(__file__).parent / "js" (recording.py:46, read at :84), but the [tool.setuptools.package-data] table only covered py.typed, **/*.j2, and the subagent prompts, so the js/ directory never reached a built wheel, and a frozen agent-server raised FileNotFoundError on browser_start_recording. The one-line addition "openhands.tools.browser_use" = ["js/*.js"] correctly closes that gap.
I verified the fix end-to-end against the workspace:
- With the fix, all six JS helpers (plus the four subagent
.mdfiles andpy.typed) are reported as declared by the test logic. - Without the fix (simulating
main), the test correctly identifies exactly the six JS files as undeclared. - The
agent-server.specalready containscollect_data_files("openhands.tools.browser_use", includes=["js/*.js"])(agent-server.spec:132), so once the wheel ships the files, the PyInstaller binary picks them up with no spec change, consistent with the PR description.
Correctness
The package-data key "openhands.tools.browser_use" resolves to the openhands/tools/browser_use/ directory, and the glob js/*.js matches all six helper files at browser_use/js/*.js. This is the correct setuptools package-data declaration.
Test Quality
The new tests/cross/test_package_data.py is a solid static regression guard. Rather than building a wheel per test run (which the PR notes would be too heavy), it walks every non-Python file under openhands/tools, skips known contributor docs (AGENTS.md, README.md), and asserts each remaining file is covered by a package-data glob. This would have caught the exact omission in #4443, and it also serves as a forward-looking guardrail: any future runtime asset added without a declaration will fail this test.
The dedicated test_browser_recording_js_helpers_are_declared is a belt-and-suspenders check that specifically asserts the browser_use/js/*.js files are covered, good for producing a focused failure message if this specific regression returns.
One minor observation (not blocking): _is_declared uses fnmatch to match patterns, but fnmatch and setuptools' pathlib.glob have slightly different semantics for **. For example, fnmatch("foo.j2", "**/*.j2") returns False while pathlib.Path.glob("**/*.j2") matches top-level files. This discrepancy is not currently exercised (there are no .j2 files under openhands/tools today, and all current patterns produce correct results), so it's purely a latent robustness note for if a top-level .j2 template is ever added.
Risk Assessment
Low risk. This is a one-line packaging declaration plus a regression test. The change is purely additive: it only causes the six JS files to be included in the built wheel; it removes nothing and changes no runtime behavior. An editable/source checkout already reads these files from the working tree, so the fix only affects wheel-based installs (which is exactly where the bug manifested). No security concerns: no untrusted input, no secrets, no new dependencies.
Verdict
No material issues. The fix is correct, minimal, and well-tested. Ship it.
HUMAN:
I first checked whether the six JS files were the only missing runtime assets, or if something else was being left out of the wheel too. Turns out js/*.js was the whole gap; everything else is either already packaged or just contributor documentation.
AGENT:
Why
Fixes #4443.
browser_use/recording.pyloads six helpers by name from_JS_DIR = Path(__file__).parent / "js"(recording.py:46, read at:84).openhands-tools/pyproject.tomldeclared package-data only forpy.typed,**/*.j2and the subagent prompts, so thejs/directory never reached the built wheel.agent-server.specalready asks for it withcollect_data_files("openhands.tools.browser_use", includes=["js/*.js"]), but there was nothing installed to collect, and a frozen agent-server raisedFileNotFoundErroronbrowser_start_recording.Nothing in a source checkout surfaces this. An editable install reads the files straight from the working tree, so the tests pass and the tool works right up until someone installs a wheel.
Summary
openhands-tools/pyproject.tomldeclares"openhands.tools.browser_use" = ["js/*.js"].tests/crosscheck walks every non-Python file underopenhands/tools, skips the contributor docs, and asserts a package-data glob covers the rest.Issue Number
Fixes #4443
How to Test
Clean wheel build, before and after. The build directory has to be removed between runs or the previous build's
build/and.egg-infoare reused and the second measurement is meaningless:On
mainthat prints[]. With this change it prints all six helpers:The new test fails on
mainnaming exactly those six files, and passes with the change:tests/cross: 327 passed, 49 skipped. Two failures are pre-existing on this Windows host and not related to this change:test_generate_baseline_payloads_uses_uv_with_release_cutofffails identically with the change stashed, andtest_websocket_attach_wait_does_not_block_ready_endpointis a 0.5s timing bound that passes on re-run.ruff format --checkandruff checkare clean on the new file.Video/Screenshots
Console output above; this path has no UI surface.
Type
Notes
openhands/toolsare the six.js, six.mdandpy.typed; of the markdown,terminal/README.mdandAGENTS.mdare contributor docs that nothing reads at runtime, and the subagent prompts already ship. Sojs/*.jsis the whole gap.openhands/tools/preset/subagents/has no__init__.py, which is why the test resolves a dotted package-data key to a directory rather than to an importable package. That matches what the wheel actually contains.agent-server.specneeds no change, as the reporter noted.