Conversation
Adds the rome-zillow OpenCLI plugin with `download` and `photos` commands that read a public Zillow home details page through the agent browser and save every gallery photo at the widest original-ratio size Zillow serves. Co-Authored-By: Claude Code <noreply@anthropic.com>
Adds opencli gemini video: opens the Gemini web video composer, attaches local images through an intercepted CDP file chooser, waits for the uploads and the render, then saves the clip with the session cookies. Reports a spent daily allowance and Gemini refusals as typed errors. Co-Authored-By: Claude Code <noreply@anthropic.com>
zoolsher
left a comment
zoolsher
left a comment
There was a problem hiding this comment.
Code Review: 💬 COMMENT
This PR adds an opencli gemini video command that drives Gemini's web video composer: it opens /videos, selects the aspect ratio, intercepts the CDP file chooser to upload reference images, submits the prompt, polls the newest model turn until a clip renders, and downloads the MP4 using the browser's session cookies. The design is thoughtful — page readers are self-contained for page.evaluate, all shaping/classification lives in pure helpers with a solid 15-test unit suite, quota-locked and refusal states are surfaced as typed errors, and cookie forwarding is correctly scoped to the media host. The code follows the existing plugin conventions (redfin/zillow) closely, and access: \"write\" is the right call since generation mutates remote account state and quota.\n\nI verified the full contents of the changed Gemini files, the tests, and the sibling plugins to confirm conventions (page.wait, getCookies, page.cdp usage all match existing plugins). No P0/P1 issues were found. The notable points are: this PR silently also contains the entire unrelated Zillow plugin (its own commit) despite being titled/scoped to Gemini; and a small mismatch between the submission-check comment and its actual enforcement. The remaining items are minor robustness/clarity nits. Note that @jackwener/opencli is not vendored in the clone, so the existence/behavior of page.bridge, page.nativeClick, and page.nativeType could not be verified from source — they are assumed correct based on the CDP-backend description.
Verdict: COMMENT — The Gemini video plugin is well-designed, self-contained, and thoroughly unit-tested with no blocking bugs or security issues; findings are a scope-discipline note and minor robustness/clarity improvements.
5 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P2 | code-quality | opencli-plugins/zillow/download.js |
PR bundles the entire unrelated Zillow plugin into a Gemini-titled change |
| P2 | error-handling | opencli-plugins/gemini/gemini-video-browser.mjs |
Submission check never enforces the conversation-URL invariant |
| P3 | code-quality | opencli-plugins/gemini/gemini-video-browser.mjs |
blob: |
| P3 | code-quality | opencli-plugins/gemini/gemini-video-helpers.mjs |
Broad error patterns matched against whole-body tail may false-positive |
| P3 | design | opencli-plugins/gemini/video.js |
--sd flag name is cryptic |
| zillowListingSlug, | ||
| } from "./zillow-helpers.mjs"; | ||
|
|
||
| cli({ |
There was a problem hiding this comment.
[P2] code-quality — PR bundles the entire unrelated Zillow plugin into a Gemini-titled change
This PR is titled/described as "add Gemini video generation plugin" and even lists what is not included, yet it also introduces the complete rome-zillow plugin (download.js, photos.js, zillow-browser.mjs, zillow-helpers.mjs, tests, README) in a separate commit. Mixing two independent features in one PR makes review and revert harder and muddies the changelog. Consider splitting the Zillow plugin into its own PR (or, if it is meant to ride along, updating the PR title/description to cover both). The Zillow code itself is well structured and tested — this is purely a scope-discipline note.
| // A accepted submission clears the editor and opens a conversation URL; a | ||
| // dropped one leaves the prompt in place, which must fail fast instead of | ||
| // waiting the whole generation timeout for a clip that never comes. | ||
| const after = await pollComposer( |
There was a problem hiding this comment.
[P2] error-handling — Submission check never enforces the conversation-URL invariant
The comment states the submission is verified to "clear the editor and open a conversation URL," but the only failure check is if (after.editorText.length > 0). The /\/app\/[a-z0-9]+/i.test(s.url) condition is used solely as a poll-exit predicate — if the editor clears but the URL never becomes a conversation (the poll then simply times out at 30s), no error is raised and the code falls through to waitForGeminiVideo, waiting the entire generation timeout for a clip that will never come. That is exactly the slow-failure the comment says it wants to avoid. Consider also asserting the conversation URL:
if (after.editorText.length > 0 || !/\/app\/[a-z0-9]+/i.test(after.url)) {
throw new CommandExecutionError("Gemini did not accept the prompt; the composer still holds it or no conversation opened");
}| } finally { | ||
| await page.cdp("Page.setInterceptFileChooserDialog", { enabled: false }).catch(() => undefined); | ||
| } | ||
| const wanted = before.attachments + files.length; |
There was a problem hiding this comment.
[P3] code-quality — blob: fallback can over-count existing attachments
When gem-media-attachment tiles are absent, readGeminiVideoComposer counts every img[src^="blob:"] on the page as an attachment. attachGeminiVideoImages then computes wanted = before.attachments + files.length and waits for s.attachments >= wanted. If the composer page shows any unrelated blob: image (avatar, prior media, etc.) at read time, before.attachments is inflated and the wait can spuriously time out with a misleading "shows N of M uploaded images" error. Scoping the blob scan to the composer/attachment container rather than the whole document would make the count robust.
| return cleaned.toLowerCase().endsWith(".mp4") ? cleaned : `${cleaned}.mp4`; | ||
| } | ||
|
|
||
| const ERROR_PATTERNS = [ |
There was a problem hiding this comment.
[P3] code-quality — Broad error patterns matched against whole-body tail may false-positive
geminiVideoStatusText falls back to bodyTail (the last 600 chars of document.body.innerText) when no model-response text is present, and generic patterns like /not available/i, /something went wrong/i, and /violat/i are then matched against it. Stray UI copy in the page chrome (menus, tooltips, footer) could be misread as a generation failure and raise a typed error mid-run. In practice responseText from the model turn is usually populated first so this is low-risk, but tightening the fallback to the response region would avoid a confusing false failure.
| help: "Max seconds to wait for the generation (default: 600)", | ||
| }, | ||
| { | ||
| name: "sd", |
There was a problem hiding this comment.
[P3] design — --sd flag name is cryptic
--sd ("skip download") is not self-explanatory next to the other readable options (--image, --ratio, --output, --name, --timeout). A clearer name such as --no-download (or --skip-download) would be more discoverable; the help text is good but the flag itself reads as a typo. Minor/optional.
Jessie-QingYu
left a comment
Jessie-QingYu
left a comment
There was a problem hiding this comment.
Code Review: 💬 COMMENT
This PR adds an OpenCLI gemini video command that drives the signed-in Gemini web session to generate a clip from a prompt (optionally animating uploaded reference images), waits for the render, and downloads the MP4 using the browser's session cookies. The design is thoughtful: page readers are self-contained for page.evaluate, all shaping/classification lives in pure helpers with a solid unit-test suite (15 tests), error/locked/refusal states are surfaced as typed errors, and image upload correctly gates on the direct CDP backend and forwards only host-scoped cookies for the authenticated download. The code quality is high and consistent with the sibling Zillow/Redfin plugins.\n\nMy findings are non-blocking robustness and scope items. The most notable is that the final download fetch is the one network operation in the flow with no timeout/abort, so a stalled media host can hang the command indefinitely. I also flag that the generation-error classifier uses broad regexes over a whole-body text fallback (false-positive abort risk), that the submit success guard's URL condition is not actually enforced (adding a potential 30s stall), and that the prompt-typing path uses page.nativeType without the CDP-backend guard that the image path has. Separately, the branch bundles an unrelated complete Zillow plugin (a second commit) that the PR title/description do not mention. Note: the runtime behavior against the live Gemini DOM could not be verified from source; I relied on the PR's stated live testing.
Verdict: COMMENT — The plugin is well-structured, well-documented, and thoroughly unit-tested; findings are robustness/scope concerns, none blocking.
5 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P2 | error-handling | opencli-plugins/gemini/gemini-video-browser.mjs |
Video download fetch has no timeout/abort and can hang indefinitely |
| P2 | correctness | opencli-plugins/gemini/gemini-video-helpers.mjs |
Broad error regexes over whole-page body can abort a healthy generation |
| P3 | correctness | opencli-plugins/gemini/gemini-video-browser.mjs |
Submit success guard's URL condition is not actually enforced |
| P3 | robustness | opencli-plugins/gemini/gemini-video-browser.mjs |
No-image path uses page.nativeType without the CDP-backend guard |
| P3 | scope | opencli-plugins/zillow/download.js |
PR bundles an unrelated Zillow plugin not covered by the description |
| export async function downloadGeminiVideo(page, src, filePath, { fetchImpl = fetch } = {}) { | ||
| const cookies = await page.getCookies({ url: src }); | ||
| const cookie = buildGeminiVideoCookieHeader(cookies, src); | ||
| const response = await fetchImpl(src, { |
There was a problem hiding this comment.
[P2] error-handling — Video download fetch has no timeout/abort and can hang indefinitely
Every other step in this flow is bounded by an explicit deadline (pollComposer, waitForGeminiVideo), and the sibling Zillow download uses httpDownload(..., { timeout: 60000 }). This final fetchImpl(src, ...) has no timeout and no AbortController, so if contribution-rt.usercontent.google.com accepts the connection but stalls the body, the command hangs forever with no way to bail. Consider adding an AbortController with a timeout (and passing signal) so a stalled media host fails cleanly like the rest of the pipeline.
| const GENERATING_PATTERNS = [/generating your video/i, /could take a few minutes/i]; | ||
|
|
||
| /** The part of the page that describes the current turn, without the sidebar. */ | ||
| export function geminiVideoStatusText(snapshot) { |
There was a problem hiding this comment.
[P2] correctness — Broad error regexes over whole-page body can abort a healthy generation
During the pending phase responseText is empty, so geminiVideoStatusText falls back to bodyTail (the last 600 chars of the entire document.body.innerText), and ERROR_PATTERNS includes very generic copy like /not available/i, /something went wrong/i, and /violat/i. Any unrelated UI text in that tail (a transient toast, a region/feature notice) would be classified as error and abort a run that would otherwise succeed. Consider scoping the error scan to the current model-response text only, and/or tightening the generic patterns.
| // A accepted submission clears the editor and opens a conversation URL; a | ||
| // dropped one leaves the prompt in place, which must fail fast instead of | ||
| // waiting the whole generation timeout for a clip that never comes. | ||
| const after = await pollComposer( |
There was a problem hiding this comment.
[P3] correctness — Submit success guard's URL condition is not actually enforced
The poll predicate requires both editorText.length === 0 and /\/app\/[a-z0-9]+/i.test(url), but the only post-poll assertion is if (after.editorText.length > 0). If a video conversation URL is not under /app/... (the composer starts at /videos), the predicate never matches, so the poll always burns the full 30s before proceeding — and the run still succeeds because the URL is never actually checked. Either drop the URL from the predicate or assert on it, so the promised fast-fail is real and the 30s stall is avoided.
| export async function submitGeminiVideoPrompt(page, prompt) { | ||
| const focused = await page.evaluate(SCRIPT_FOCUS_EDITOR); | ||
| if (!focused) throw new CommandExecutionError("Gemini prompt editor was not found"); | ||
| await page.nativeType(prompt); |
There was a problem hiding this comment.
[P3] robustness — No-image path uses page.nativeType without the CDP-backend guard
attachGeminiVideoImages explicitly requires the direct CDP backend (checks page.cdp/page.bridge) before using nativeClick, but submitGeminiVideoPrompt calls page.nativeType(prompt) unconditionally. If native input is likewise unavailable on the Browser Bridge backend, a run without --image would fail here with an opaque error rather than the clean typed guidance the PR says is the only Bridge-reachable outcome. Worth confirming nativeType works on the Bridge, or gating the whole command on the CDP backend to match the README's CDP-only examples.
| @@ -0,0 +1,102 @@ | |||
| import * as fs from "node:fs"; | |||
There was a problem hiding this comment.
[P3] scope — PR bundles an unrelated Zillow plugin not covered by the description
The branch contains two commits — the Gemini video plugin and a full Zillow listing-photo plugin (7 files) — but the PR title and description only cover Gemini. Mixing two independent features in one PR hurts reviewability and bisecting. If this isn't intentional stacking, consider splitting the Zillow plugin into its own PR. (The Zillow code itself is clean and well-tested; this is purely a scope note.)
yunfanye
left a comment
yunfanye
left a comment
There was a problem hiding this comment.
Code Review: 🛑 REQUEST_CHANGES
This PR adds a Gemini video-generation OpenCLI command using the existing flat plugin architecture, with CDP file-chooser interception, polling, authenticated download, and pure helper tests. The Gemini and Zillow helper suites both pass (15 and 24 tests), and CI is green, but the submission/state handling contains failure-path bugs that can turn fast typed errors into full generation timeouts; the PR also accidentally bundles a separate 1,531-line Zillow feature.
Verdict: REQUEST_CHANGES — The Gemini flow does not reliably reject failed submissions or page-level generation errors, so common failure states can wait for the full timeout instead of failing as designed.
3 finding(s) posted as inline comments below.
| Severity | Category | File | Title |
|---|---|---|---|
| P1 | error-handling | opencli-plugins/gemini/gemini-video-browser.mjs |
Require the conversation URL before accepting submission |
| P2 | error-handling | opencli-plugins/gemini/gemini-video-helpers.mjs |
Fall back when the model response is empty |
| P2 | architecture | opencli-plugins/README.md |
Split the unrelated Zillow feature from this PR |
| (s) => s.editorText.length === 0 && /\/app\/[a-z0-9]+/i.test(s.url), | ||
| { timeoutMs: 30000 }, | ||
| ); | ||
| if (after.editorText.length > 0) { |
There was a problem hiding this comment.
[P1] error-handling — Require the conversation URL before accepting submission
pollComposer requires both an empty editor and an /app/<id> URL, but on timeout this code only checks whether the editor still has text. If Gemini clears the editor yet remains on /videos, the method reports success and waits up to the full generation timeout—the exact dropped/failed-submission path this check is meant to catch. Validate both postconditions after polling and add a test for an empty editor with no conversation URL.
|
|
||
| /** The part of the page that describes the current turn, without the sidebar. */ | ||
| export function geminiVideoStatusText(snapshot) { | ||
| const text = String(snapshot?.responseText ?? snapshot?.bodyTail ?? ""); |
There was a problem hiding this comment.
[P2] error-handling — Fall back when the model response is empty
readGeminiVideoTurn always supplies responseText as a string, so when there is no model-response it is "" and the nullish coalescing here never reads bodyTail. Page-level failures such as “Something went wrong” are consequently classified as pending and run until timeout. Select the first non-empty status source and cover the actual { responseText: "", bodyTail: "..." } snapshot shape.
| - `opencli redfin download URL [--output DIR] [--size SIZE] [--limit N]` — downloads every gallery | ||
| photo of a public Redfin listing into one folder per listing, in gallery order, with captions and | ||
| room tags in the result rows. `photos URL` lists the same gallery without writing anything. | ||
| - `opencli zillow download URL [--output DIR] [--size SIZE] [--limit N]` — downloads every gallery |
There was a problem hiding this comment.
[P2] architecture — Split the unrelated Zillow feature from this PR
The PR includes the complete Zillow download/photos plugin (1,531 added lines across seven files) even though its title, description, invariants, and test plan are for Gemini video generation. This couples independent review and rollback risk to the Gemini change. Remove commit 5a8768531585 from this branch and submit the Zillow plugin separately.
3 participants
What this PR does
The Gemini adapter could ask questions and generate images, but the web app's video composer had no command, and nothing could hand it a photo to animate. Turning a listing's stills into short cinematic clips therefore meant driving the page by hand every time.
opencli gemini video PROMPT [--image A.jpg,B.jpg] [--ratio 16:9|9:16] [--output DIR] [--name FILE] [--timeout S] [--sd]opensgemini.google.com/videosin the signed-in session, picks the aspect ratio, attaches the images, submits the prompt, polls the newest model turn until a player renders, and saves the MP4.Design & Invariants
/videospre-selects Gemini's Videos tool, and the command fails when that chip is absent rather than guessing at the tools menu.Page.setInterceptFileChooserDialog+DOM.setFileInputFiles), so image upload requires the direct--cdp-endpointbackend and says so when run through the Browser Bridge.contribution-rt.usercontent.google.com) redirects anonymous requests to sign-in. The download forwards only the browser cookies whose domain covers that host and refuses any non-video response.Test plan
npm testinopencli-plugins/gemini(15 tests)biome check opencli-plugins/geminifrom the repo root, cleandurationreported)Not in this PR
🤖 Generated with Claude Code