diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b0ec9a2b..9c2187e79 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`video chain --out-file PATH` (and the deprecated `-o`) now writes the clips there.** It was + accepted and ignored. Link i's clip goes to `_` next to PATH + (`out/clip.mp4` gives `out/clip_1.mp4`, `out/clip_2.mp4`), numbered over the whole manifest so + `--resume-from` keeps each link's name; `--json`, the chain table and the videos catalog report + those paths, and seed frames stay under `--out-dir`. An existing file or folder at a numbered + path is never replaced, and a clip whose move fails is never lost: either way it stays where it + downloaded, is recorded there, and a warning names the link, its media id and the path. A + FILE under an existing file is a usage error before the cost prompt. The stderr notice is gone + ([#1293](https://github.com/ffroliva/gflow-cli/issues/1293)). +- **`video chain --resume-from` continues the chain instead of restarting it.** The first resumed + link was numbered 0: it overwrote link 0's chain row, went out as an unseeded text-to-video, + and left the chain one row short, so a later resume billed a finished link again. Resumed links + now keep their manifest index (chain table, `--json` `links[].index`, `--out-file` number), the + first one is an image-to-video seeded from the last completed clip, and a resume whose clip is + gone is refused with exit 20 naming the file, before anything is billed + ([#1300](https://github.com/ffroliva/gflow-cli/issues/1300)). + - **`gflow update` says one thing about the result ([#1163](https://github.com/ffroliva/gflow-cli/issues/1163)).** It no longer shows the cached "update available" banner, which could name an older release than the one it then diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 2253d5eee..60a5e5f27 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -639,11 +639,12 @@ waits for each link in turn, so total wallclock is the sum of all link waits. | `--max-links N` | unset | Cap link count; exit 11 (`ConfigurationError`) if the manifest has more. A spend guardrail. | | `-y` / `--yes` | off | Skip the pending video operation confirmation prompt. | | `--dry-run` | off | Print the pending video operation plan and submit nothing. | -| `--resume-from CHAIN_ID` | unset | Resume a prior chain by its id; already-completed links are skipped (not regenerated). | +| `--resume-from CHAIN_ID` | unset | Resume a prior chain by its id; already-completed links are skipped (not regenerated), and the next link is an i2v seeded from the last completed clip, which must still be at its recorded path (exit 20 otherwise, before anything is billed; [#1300](https://github.com/ffroliva/gflow-cli/issues/1300)). | | `--jitter F` | `0.0` | Random `0..F` second pause **between** links (anti-bot cadence; never before link 0). | | `--seed-offset MS` | `0` | Extract the seed frame this many ms before EOF (fade-to-black guard). | | `--aspect` | `9:16` | `9:16` / `16:9`. Applied uniformly to every link (continuity requirement). | -| `--out-dir DIR` | output root | Directory for the link mp4s + `linkN_lastframe.jpg` seed frames. | +| `--out-file FILE` | unset | Link i's clip goes to `_` next to FILE (`clip.mp4` gives `clip_1.mp4`, `clip_2.mp4`), numbered over the whole manifest so `--resume-from` keeps each name; seed frames stay under `--out-dir`. An existing file or folder there is never replaced: that clip, like one whose move fails, stays where it downloaded, recorded there, with a warning. A FILE under an existing file is a usage error (exit 2) before anything runs. Deprecated spelling: `-o/--output` ([#1293](https://github.com/ffroliva/gflow-cli/issues/1293)). | +| `--out-dir DIR` | output root | Directory for the `linkN_lastframe.jpg` seed frames, and for the link mp4s unless `--out-file` is given. | | `--profile NAME` | default profile | Per-subcommand profile override. | | `--json` | off | Emit a machine-readable JSON result. | diff --git a/docs/PROJECT_STATUS.md b/docs/PROJECT_STATUS.md index 3720eadb7..fad82f5e2 100644 --- a/docs/PROJECT_STATUS.md +++ b/docs/PROJECT_STATUS.md @@ -20,8 +20,8 @@ rate limit, content policy, network, wire format and timeout to `40`–`44` and into `3`. The default stays the legacy codes; `--json` errors carry both `exit_code` and `exit_code_legacy`. The table is in [ERRORS.md](ERRORS.md). -**Changed.** `video chain` has never written to `-o PATH`; it now says so on stderr instead of staying -silent (#1293 tracks writing the file). +**Changed.** In this release `video chain` does not write to `-o PATH` and says so on stderr instead of +staying silent; writing the file is fixed on `develop` for the next release (#1293). Internally, `UiAutomationTransport` and `MigratedComposer` are split into single-purpose mixins (#1249 wave 2). diff --git a/docs/USAGE.md b/docs/USAGE.md index 509331c10..3b6fe3232 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -735,8 +735,8 @@ All prompts in a batch share one Flow project. The editor is opened once and sta > instead of proceeding on Flow's sticky default (typically x2) and silently > changing what the run bills. > `t2v`, `i2v`, `r2v`, `video chain`, and `video extend` additionally take `--out-file PATH` (explicit file destination). -> `video chain` does not apply it yet: it says so on stderr and the clips land under `--out-dir` ([#1293](https://github.com/ffroliva/gflow-cli/issues/1293)). -> The mp4 lands at `` or `/.mp4`. +> `video chain` writes link i's clip to `_` next to PATH (`--out-file out/clip.mp4` gives `out/clip_1.mp4`, `out/clip_2.mp4`, …), numbered over the whole manifest even for one link, so `--resume-from` writes the remaining links under the names a full run would; seed frames stay under `--out-dir`, and `--json` and the catalog report the numbered paths. An existing file or folder at a numbered path is never replaced: that clip, like one whose move fails, stays at its `--out-dir` path, is recorded there, and a warning names it ([#1293](https://github.com/ffroliva/gflow-cli/issues/1293)). +> For `t2v`, `i2v` and `r2v` the mp4 lands at `` or `/.mp4`. > > **`i2v` model rules (issues #125 / #626):** every model — `omni-flash` > included — supports i2v with a **start frame** and with an **end frame** @@ -1191,14 +1191,19 @@ Options: --dry-run Resolve the manifest, print the plan + credit cost, and spend nothing. --resume-from CHAIN_ID Resume a prior chain by its id; already-paid links - are skipped (not re-billed). + are skipped (not re-billed), and the next link is + seeded from the last paid clip, which must still be + at its recorded path (exit 20 otherwise). --jitter FLOAT Random 0..JITTER second pause between links (anti-bot cadence). [default: 0.0] --seed-offset INTEGER Extract the seed frame this many ms before EOF (fade-to-black guard). [default: 0] --aspect [9:16|16:9] Uniform aspect for every link. [default: 9:16] --profile NAME Profile name (overrides default). - --out-dir DIR Directory for the link mp4s + seed frames. + --out-file FILE Write link i's clip to FILE's stem plus _i + (clip.mp4 -> clip_1.mp4, clip_2.mp4). + --out-dir DIR Directory for the seed frames, and for the link + mp4s unless --out-file is given. --json Emit a machine-readable JSON result. ``` @@ -1237,7 +1242,10 @@ the manifest is currently informational). ### Output — N clips, not one file Each link is saved as its own mp4 (plus a `linkN_lastframe.jpg` seed frame -between links) under `--out-dir` (or the default output dir). **Stitching the +between links) under `--out-dir` (or the default output dir). With +`--out-file out/clip.mp4` link i's clip is written to `out/clip_.mp4` +instead (1-based over the whole manifest, so a `--resume-from` run keeps each +link's number); the seed frames stay under `--out-dir`. **Stitching the clips into a single video is a separate step — use `gflow scene` to concatenate them server-side.** Chain does not auto-concat: see the *deferred auto-concat* entry in @@ -2372,7 +2380,7 @@ The table below is the default (legacy) set. `GFLOW_CLI_EXIT_CODES=2` opts in to | `17` | `ModelModeIncompatibilityError` | The chosen video model can't do the requested mode — today that is `omni-flash` or `omni-1.2-flash` for `chain` (issues #125, #626) | Use a Veo 3.1 model (`veo-lite` / `veo-fast` / `veo-quality` / `veo-lite-lp`) for `chain`. Single-clip `i2v` with omni-flash, `--end-frame` included, is accepted | | `18` | `VideoModelSelectionError` | gflow could not select the requested video model in Flow's editor for an `i2v` run (model-picker option not found) | Usually transient — retry; if it persists, Flow's model-picker UI changed (report referencing #125) | | `19` | `SceneConcatError` | Server-side scene render/concat failed (`gflow scene --out-file`) | Retry; the recorded compose survives, so re-render is safe | -| `20` | `FrameExtractionError` | Could not extract the last frame for a video chain link | Check the source video downloaded intact; retry the link | +| `20` | `FrameExtractionError` | Could not extract the last frame for a video chain link, or a `video chain --resume-from` whose last completed clip is no longer at its recorded path (refused before anything is billed, [#1300](https://github.com/ffroliva/gflow-cli/issues/1300)) | Check the source video downloaded intact; retry the link. For a resume, put the clip back at the path named, or start a new chain | | `21` | `ChainPartialError` | A video chain stopped mid-way; earlier links completed | Resume from the last completed link shown in the error | | `22` | `UpscaleUnavailableError` | 4K image upscale is gated to Flow **Ultra** accounts; or requested video export quality is disabled | For images, use `--scale 2k` or upgrade the Flow plan; for videos, check available options in Flow | | `23` | `UiSelectorDriftError` | A Flow editor control could not be located — Google changed the frontend (issues #183, #493) — **or** a blocking announcement overlay survived dismissal, so no control below it can be clicked (probe `overlay_close_button`, #593). On the migrated host a missing submit control is checked against the wallet first and reported as **37** when Flow swapped in its insufficient-credits warning, so a short balance no longer arrives here | If the detail says the model picker is **hidden**, nothing was submitted: turn the **Agent** chip off in a browser and re-run ([#788](https://github.com/ffroliva/gflow-cli/issues/788)). Otherwise update gflow-cli; file a bug with the probe name + the diagnostics JSON / debug screenshot referenced in the error message, plus the incident bundle's `report.md` when one was written. For `overlay_close_button`: open the project once in Chrome and dismiss the announcement — the dismissal persists on your account | diff --git a/plugins/gflow/skills/gflow-cli/SKILL.md b/plugins/gflow/skills/gflow-cli/SKILL.md index 39ddedb50..33911e08e 100644 --- a/plugins/gflow/skills/gflow-cli/SKILL.md +++ b/plugins/gflow/skills/gflow-cli/SKILL.md @@ -112,7 +112,7 @@ gflow video upscale --scale {1080p|720p|270p} [--project ID] [--out DI # removed. For multi-clip runs, loop `gflow video t2v`/`i2v` from the shell. gflow video chain [--out-dir DIR] [--dry-run] \ [--max-links N] [--resume-from N] # last-frame I2V chaining; veo models only - # --out-file is not applied by chain yet (stderr notice, #1293): clips go to --out-dir + # --out-file out/clip.mp4 writes link i to out/clip_.mp4 (#1293) # Characters (reusable, project-scoped subjects) gflow character create --project --name "" --face-prompt "" \ diff --git a/skills/gflow-cli/SKILL.md b/skills/gflow-cli/SKILL.md index 39ddedb50..33911e08e 100644 --- a/skills/gflow-cli/SKILL.md +++ b/skills/gflow-cli/SKILL.md @@ -112,7 +112,7 @@ gflow video upscale --scale {1080p|720p|270p} [--project ID] [--out DI # removed. For multi-clip runs, loop `gflow video t2v`/`i2v` from the shell. gflow video chain [--out-dir DIR] [--dry-run] \ [--max-links N] [--resume-from N] # last-frame I2V chaining; veo models only - # --out-file is not applied by chain yet (stderr notice, #1293): clips go to --out-dir + # --out-file out/clip.mp4 writes link i to out/clip_.mp4 (#1293) # Characters (reusable, project-scoped subjects) gflow character create --project --name "" --face-prompt "" \ diff --git a/src/gflow_cli/chain.py b/src/gflow_cli/chain.py index b136c7245..324f55b8d 100644 --- a/src/gflow_cli/chain.py +++ b/src/gflow_cli/chain.py @@ -54,7 +54,7 @@ from gflow_cli.media import extract_last_frame if TYPE_CHECKING: - from collections.abc import Sequence + from collections.abc import Callable, Sequence from pathlib import Path from gflow_cli.api.client import FlowApiClient @@ -325,6 +325,46 @@ def _build_link_result( ) +async def _relocate_guarded( + relocate: Callable[[int, VideoResult], VideoResult], index: int, result: VideoResult +) -> VideoResult: + """Run the ``--out-file`` move off the loop; on any failure keep the clip where it is. + + The clip is already paid for, so it must reach the recorder whatever the hook does: a + link without a chain row is billed again on ``--resume-from``. Same rule as + :func:`_notify_link_failed`. + """ + try: + return await asyncio.to_thread(relocate, index, result) + except Exception as exc: # noqa: BLE001 — see docstring + _log.warning( + "chain_clip_kept_at_download_path", + link=index + 1, + media_id=result.status.media_id, + kept_at=str(result.local_path), + reason=str(exc), + ) + return result + + +async def _resume_seed( + extractor: FrameExtractor, + *, + start_index: int, + seed_clip: Path | None, + out_dir: Path, + offset_ms: int, +) -> Path | None: + """The seed frame for ``links[0]``: none at link 0, else re-extracted from ``seed_clip``.""" + if start_index == 0: + return None + if seed_clip is None: + msg = f"link {start_index} continues a chain: pass seed_clip, the clip of the link before" + raise ValueError(msg) + dst = out_dir / f"link{start_index - 1}_lastframe.jpg" + return await asyncio.to_thread(extractor, src=seed_clip, dst=dst, offset_ms=offset_ms) + + async def run_chain( *, client: FlowApiClient, @@ -339,6 +379,9 @@ async def run_chain( aspect: Aspect = Aspect.PORTRAIT, seed_offset_ms: int = 0, jitter: float = 0.0, + relocate: Callable[[int, VideoResult], VideoResult] | None = None, + start_index: int = 0, + seed_clip: Path | None = None, ) -> list[ChainLinkResult]: """Render ``links`` as a sequential last-frame I2V chain. @@ -368,6 +411,16 @@ async def run_chain( before EOF (the fade-to-black mitigation). jitter: When > 0, sleep a random ``[0, jitter)`` seconds BETWEEN links (anti-bot cadence); never before link 0. + relocate: Optional ``(index, result) -> result`` that moves a downloaded + clip (``--out-file``, #1293). It runs off the event loop before + anything reads the path, so the recorders, the seed extraction and + the return value all see where the clip ended up. If it raises, the + clip stays at its download path and a warning says so. A result with + no ``local_path`` must come back unchanged. + start_index: The manifest index of ``links[0]`` (``--resume-from``, #1300). Request + mode, ``ChainLinkResult.index`` and the recorded ``link_index`` all count from it. + seed_clip: The clip of link ``start_index - 1``; its last frame seeds ``links[0]``. + Required when ``start_index`` is past 0, so a resumed link is never sent as t2v. Returns: One :class:`ChainLinkResult` per link, in order. @@ -380,18 +433,26 @@ async def run_chain( ChainPartialError: A link failed with a ``WireFormatError`` (i2v routed to the t2v backstop) or ``WafRejectionError`` (403). Carries the ``Path`` of every link completed before the failure. + ValueError: ``start_index`` is past 0 but no ``seed_clip`` was given. """ reject_unusable_links(model=model, links=links) results: list[ChainLinkResult] = [] completed_paths: list[Path] = [] - prev_frame: Path | None = None + prev_frame = await _resume_seed( + extractor, + start_index=start_index, + seed_clip=seed_clip, + out_dir=out_dir, + offset_ms=seed_offset_ms, + ) + last_index = start_index + len(links) - 1 - for index, spec in enumerate(links): - if index > 0 and jitter > 0: + for index, spec in enumerate(links, start=start_index): + if index > start_index and jitter > 0: await asyncio.sleep(random.uniform(0.0, jitter)) # noqa: S311 # cadence, not crypto - _log.info("chain_link_started", index=index, total_links=len(links)) + _log.info("chain_link_started", index=index, total_links=last_index + 1) req = _build_link_request( spec=spec, index=index, model=model, aspect=aspect, prev_frame=prev_frame ) @@ -405,6 +466,8 @@ async def run_chain( on_link_started=on_link_started, on_link_failed=on_link_failed, ) + if relocate is not None: # a missing clip is left as is and refused just below + result = await _relocate_guarded(relocate, index, result) if result.local_path is None: msg = f"link {index} returned no local_path (download failed)" @@ -419,7 +482,7 @@ async def run_chain( _notify_link_failed(on_link_failed, req, exc) raise exc - is_last = index == len(links) - 1 + is_last = index == last_index # RECORD-BEFORE-EXTRACT: persist the downloaded clip before decoding it. # The frame_path is the planned seed-frame destination; it is filled in diff --git a/src/gflow_cli/cli_image.py b/src/gflow_cli/cli_image.py index 3a175b91d..e7c7b068d 100644 --- a/src/gflow_cli/cli_image.py +++ b/src/gflow_cli/cli_image.py @@ -110,7 +110,7 @@ from gflow_cli.image_batch import ( MIN_COUNT as _MIN_COUNT, ) -from gflow_cli.paths import image_output_path, resolve_batch_output_dir +from gflow_cli.paths import image_output_path, numbered_path, resolve_batch_output_dir from gflow_cli.profile_store import account_locale_for from gflow_cli.refresh_resolver import ( _build_name_resolver as _build_name_resolver, # pyright: ignore[reportPrivateUsage] @@ -1127,7 +1127,7 @@ async def _download_images( if len(images) == 1: target = output_file else: - target = output_file.parent / f"{output_file.stem}_{i}{output_file.suffix}" + target = numbered_path(output_file, i) elif out is not None: target = out / f"{img.media_name}_{i}.png" else: diff --git a/src/gflow_cli/cli_video.py b/src/gflow_cli/cli_video.py index f31ad7c1a..fc747f89a 100644 --- a/src/gflow_cli/cli_video.py +++ b/src/gflow_cli/cli_video.py @@ -54,12 +54,17 @@ SceneConcatError, ) from gflow_cli.image_batch import resolve_jitter_range -from gflow_cli.paths import extend_output_path +from gflow_cli.paths import ( + chain_clip_relocator, + extend_output_path, + relocate_video_output, +) from gflow_cli.storage import cloud_info_from_path, refuse_cloud_storage_for_local_file if TYPE_CHECKING: from collections.abc import Callable, Sequence + from gflow_cli.api.video import VideoResult from gflow_cli.api.video_extend import ExtendStarted from gflow_cli.chain import ChainLinkSpec from gflow_cli.tools.invocation import AppliedTool @@ -240,44 +245,6 @@ def _reject_duration_without_control( raise click.UsageError(f"{named}: {exc}") from exc -def _relocate_single_video(item: Any, target: Path) -> Any: - from dataclasses import replace - from typing import cast - - local_p = cast("Path | None", getattr(item, "local_path", None)) - if local_p is None or not local_p.exists(): - return item - if local_p != target: - import shutil - - # shutil.move, not Path.replace: a rename across mounts raises EXDEV (#1114). - shutil.move(local_p, target) - return replace(item, local_path=target) - return item - - -def _relocate_video_output(result: Any, output_file: Path | None) -> Any: - if output_file is None: - return result - from typing import cast - - output_file.parent.mkdir(parents=True, exist_ok=True) - if isinstance(result, (list, tuple)): - items = cast("tuple[Any, ...] | list[Any]", result) - relocated: list[Any] = [] - is_single = len(items) == 1 - for i, item in enumerate(items, start=1): - target = ( - output_file - if is_single - else output_file.parent / f"{output_file.stem}_{i}{output_file.suffix}" - ) - relocated.append(_relocate_single_video(item, target)) - return relocated - - return _relocate_single_video(result, output_file) - - async def _generate_and_report( request: Any, *, @@ -367,7 +334,7 @@ def on_started(started: VideoStarted) -> None: **resolver_kw, ) - result = _relocate_video_output(result, output_file) + result = relocate_video_output(result, output_file) try: recorder.record_completed_video( @@ -745,21 +712,23 @@ def _resolve_chain_resume( settings: Any, profile_name: str, profile_dir: Path, -) -> tuple[str, int]: - """Resolve the chain_id and number of already-completed links. +) -> tuple[str, int, Path | None]: + """Resolve the chain_id, the number of already-completed links and the last one's clip. For a fresh run mints a new UUID. For a resume, opens the chain recorder to count completed links and returns early (raises SystemExit via console.print + return sentinel) when the chain is already done. - Returns ``(chain_id, skipped)`` — callers check ``skipped >= len(links)`` - themselves via the returned value; this helper raises nothing. + Returns ``(chain_id, skipped, seed_clip)`` — callers check ``skipped >= len(links)`` + themselves. ``seed_clip`` seeds the first remaining link (#1300); a resume whose clip + is gone is refused here, before the cost prompt, rather than sent as an unseeded t2v. """ import uuid from gflow_cli.data.chain_repo import ChainLinkRecorder + from gflow_cli.errors import FrameExtractionError if resume_from is None: - return str(uuid.uuid4()), 0 + return str(uuid.uuid4()), 0, None chain_id = resume_from probe = ChainLinkRecorder.open( @@ -769,10 +738,18 @@ def _resolve_chain_resume( chain_id=chain_id, ) try: - skipped = len(probe.completed_links()) + completed = probe.completed_links() finally: probe.close() - return chain_id, skipped + seed_clip = Path(completed[-1].local_path) if completed else None + if seed_clip is not None and len(completed) < len(links) and not seed_clip.is_file(): + raise FrameExtractionError( + detail=f"cannot resume chain {chain_id}: the clip of its last completed link, " + f"{seed_clip}, is missing, so the next link has nothing to continue from", + remediation_hint="Put the clip back at that path, or start a new chain.", + charged="no", + ) + return chain_id, len(completed), seed_clip @dataclass(frozen=True) @@ -790,6 +767,9 @@ class _ChainExecConfig: jitter: float chain_id: str as_json: bool + relocate: Callable[[int, VideoResult], VideoResult] | None = None + start_index: int = 0 + seed_clip: Path | None = None async def _execute_chain_links( @@ -817,7 +797,7 @@ async def _execute_chain_links( chain_id = cfg.chain_id as_json = cfg.as_json - from gflow_cli.api.video import GenerateVideoRequest, VideoResult, VideoStarted + from gflow_cli.api.video import GenerateVideoRequest, VideoStarted from gflow_cli.errors import ChainPartialError # (request, media_id) pairs — matched by object IDENTITY in _on_link_failed. @@ -899,6 +879,9 @@ def _on_link_completed(request: GenerateVideoRequest, result: VideoResult) -> No aspect=aspect_enum, seed_offset_ms=seed_offset, jitter=jitter, + relocate=cfg.relocate, + start_index=cfg.start_index, + seed_clip=cfg.seed_clip, ) return results, False, completed_paths except ChainPartialError as exc: @@ -1024,7 +1007,7 @@ async def _run_chain( # Resume: bind the prior chain_id and skip completed links so they are not # regenerated. A fresh run mints a new id. - chain_id, skipped = _resolve_chain_resume( + chain_id, skipped, seed_clip = _resolve_chain_resume( resume_from, links, settings=settings, @@ -1109,6 +1092,9 @@ async def _run_chain( jitter=jitter, chain_id=chain_id, as_json=as_json, + relocate=chain_clip_relocator(output_file), + start_index=skipped, + seed_clip=seed_clip, ), ) finally: @@ -1788,13 +1774,15 @@ def r2v( ) @click.option("--profile", default=None, help="Profile name (overrides default).") @tool_option -@out_file_option(help="Explicit output file path for the generated chain clips.") +@out_file_option( + help="Write link i's clip to FILE's stem plus _i (clip.mp4 -> clip_1.mp4, clip_2.mp4)." +) @click.option( "--out-dir", "out_dir", default=None, type=click.Path(file_okay=False, path_type=Path), - help="Directory to save the link mp4s + seed frames. Defaults to the output dir.", + help="Directory for the seed frames, and the link mp4s unless --out-file is given.", ) @click.option( "--json", @@ -1819,11 +1807,11 @@ def chain( as_json: bool, ) -> None: """Render the chain MANIFEST (one continuous last-frame I2V sequence).""" - if output_file is not None: # _run_chain has never written there; say so (#1293) - click.echo( - "video chain does not apply --out-file yet (#1293); clips are saved under --out-dir.", - err=True, - ) + # Before the cost prompt and the browser: the clips' folder must be creatable. + blocker = next((p for p in output_file.parents if p.exists()), None) if output_file else None + if blocker is not None and not blocker.is_dir(): + msg = f"{blocker} is a file, so {output_file} cannot be written under it." + raise click.BadParameter(msg, param_hint="'--out-file'") profile_name = _resolve_profile(profile) provider_dir = _make_provider_dir(profile_name) run_with_handlers( diff --git a/src/gflow_cli/paths.py b/src/gflow_cli/paths.py index 288e7eea4..7459e0e2e 100644 --- a/src/gflow_cli/paths.py +++ b/src/gflow_cli/paths.py @@ -20,12 +20,19 @@ from __future__ import annotations import re +import shutil +from collections.abc import Callable +from dataclasses import replace from datetime import date from pathlib import Path +from typing import TYPE_CHECKING, Any, cast from uuid import uuid4 from platformdirs import user_data_dir, user_downloads_dir +if TYPE_CHECKING: + from gflow_cli.api.video import VideoResult + # Allow only alphanumerics, hyphens, and underscores up to 128 chars. # Prevents path traversal via API-returned job IDs. _SAFE_ID_RE = re.compile(r"^[\w\-]{1,128}$") @@ -288,3 +295,67 @@ def correct_image_extension(path: Path) -> Path: return path path.rename(target) return target + + +def numbered_path(path: Path, number: int) -> Path: + """File ``number`` (1-based) of several written from one ``--out-file PATH``. + + The one naming rule for every command that fans one ``--out-file`` out to several + files (``hero.png`` -> ``hero_1.png``, ``hero_2.png``; #1158, #1293). + """ + return path.parent / f"{path.stem}_{number}{path.suffix}" + + +def relocate_video(item: Any, target: Path) -> Any: + """Move ``item.local_path`` to ``target``; ``item`` unchanged when there is no file.""" + local_p = cast("Path | None", getattr(item, "local_path", None)) + if local_p is None or not local_p.exists(): + return item + if local_p != target: + # shutil.move, not Path.replace: a rename across mounts raises EXDEV (#1114). + shutil.move(local_p, target) + return replace(item, local_path=target) + return item + + +def relocate_video_output(result: Any, output_file: Path | None) -> Any: + """Apply ``--out-file`` to one result or a list: one file keeps PATH, several are numbered.""" + if output_file is None: + return result + output_file.parent.mkdir(parents=True, exist_ok=True) + if isinstance(result, (list, tuple)): + items = cast("tuple[Any, ...] | list[Any]", result) + relocated: list[Any] = [] + is_single = len(items) == 1 + for i, item in enumerate(items, start=1): + target = output_file if is_single else numbered_path(output_file, i) + relocated.append(relocate_video(item, target)) + return relocated + + return relocate_video(result, output_file) + + +def chain_clip_relocator( + output_file: Path | None, +) -> Callable[[int, VideoResult], VideoResult] | None: + """``--out-file`` for a chain (#1293): link ``index`` goes to ``_``. + + ``index`` counts the whole manifest (#1300), so a resumed run names its links as a full + run would. Always numbered, even for one link, so a resume never renames a clip. Raises + instead of replacing an existing target; ``run_chain`` then keeps the clip where it is. + """ + if output_file is None: + return None + output_file.parent.mkdir(parents=True, exist_ok=True) + + def relocate(index: int, result: VideoResult) -> VideoResult: + target = numbered_path(output_file, index + 1) + if target.exists(): # an earlier run's clip, or a folder: never replace it + raise FileExistsError(f"{target} already exists") + try: + return relocate_video(result, target) + except OSError: + target.unlink(missing_ok=True) # a partial copy: the check above saw no file here + raise + + return relocate diff --git a/src/gflow_cli/worker/daemon.py b/src/gflow_cli/worker/daemon.py index 79bf7afda..6dbc5234a 100644 --- a/src/gflow_cli/worker/daemon.py +++ b/src/gflow_cli/worker/daemon.py @@ -31,7 +31,7 @@ outcome_members, ) from gflow_cli.observability import exception_message_hash -from gflow_cli.paths import image_output_path +from gflow_cli.paths import image_output_path, numbered_path from gflow_cli.services.reference_uploads import generate_reusing_uploads from gflow_cli.storage import cloud_info_from_path from gflow_cli.worker import codec @@ -269,10 +269,7 @@ async def _generate(request: GenerateImageRequest) -> list[GeneratedImage]: if len(images) == 1: target = output_file else: - target = ( - output_file.parent - / f"{output_file.stem}_{i}{output_file.suffix}" - ) + target = numbered_path(output_file, i) target.parent.mkdir(parents=True, exist_ok=True) else: target = image_output_path( diff --git a/tests/cli/test_cli_video.py b/tests/cli/test_cli_video.py index 1d900943e..26cf480a9 100644 --- a/tests/cli/test_cli_video.py +++ b/tests/cli/test_cli_video.py @@ -1729,7 +1729,7 @@ def test_duration_on_omni_flash_passes_the_guard(self, tmp_path: Path) -> None: def test_output_rejects_an_existing_directory_before_generating(tmp_path: Path, sub: str) -> None: """`-o ` must fail at parse time, not after a paid generation. - `_relocate_video_output` calls `Path.replace()` onto the target, which raises + `relocate_video_output` calls `Path.replace()` onto the target, which raises `PermissionError: [WinError 5]` when the target is a directory. That happens AFTER Flow has rendered and billed the clip, so it surfaced as a bare "Unexpected error" exit 1 and left the paid mp4 orphaned under a UUID name at diff --git a/tests/cli/test_cli_video_chain.py b/tests/cli/test_cli_video_chain.py index e548a67a0..a8738cb1a 100644 --- a/tests/cli/test_cli_video_chain.py +++ b/tests/cli/test_cli_video_chain.py @@ -61,6 +61,16 @@ def _fake_link_result(index: int, tmp_path: Path) -> Any: ) +def _done_links(tmp_path: Path, n: int) -> list[Any]: + """Recorded links whose clips are on disk, so a resume can seed from the last (#1300).""" + links = [] + for i in range(n): + clip = tmp_path / f"done{i}.mp4" + clip.touch() + links.append(MagicMock(link_index=i, local_path=str(clip))) + return links + + def _assert_no_numeric_credit_claim(output: str) -> None: numeric_credit_lines = [ line @@ -138,7 +148,7 @@ def test_chain_resumed_dry_run_counts_only_remaining_operations(tmp_path: Path) runner = CliRunner() manifest = _manifest(tmp_path, 3) fake_recorder = MagicMock() - fake_recorder.completed_links.return_value = [MagicMock(), MagicMock()] + fake_recorder.completed_links.return_value = _done_links(tmp_path, 2) with ( patch("gflow_cli.cli_video._resolve_profile", return_value="default"), @@ -170,7 +180,7 @@ def test_chain_resume_with_all_links_completed_returns_before_external_work( runner = CliRunner() manifest = _manifest(tmp_path, 3) fake_recorder = MagicMock() - fake_recorder.completed_links.return_value = [MagicMock(), MagicMock(), MagicMock()] + fake_recorder.completed_links.return_value = _done_links(tmp_path, 3) with ( patch("gflow_cli.cli_video._resolve_profile", return_value="default"), @@ -462,7 +472,7 @@ def test_chain_resume_skips_completed_links(tmp_path: Path) -> None: # Recorder reports the first link already paid for. fake_recorder = MagicMock() - fake_recorder.completed_links.return_value = [MagicMock()] # 1 completed + fake_recorder.completed_links.return_value = _done_links(tmp_path, 1) fake_client = MagicMock() fake_client.__aenter__ = AsyncMock(return_value=fake_client) diff --git a/tests/cli/test_cli_video_chain_out_file.py b/tests/cli/test_cli_video_chain_out_file.py new file mode 100644 index 000000000..e27966266 --- /dev/null +++ b/tests/cli/test_cli_video_chain_out_file.py @@ -0,0 +1,279 @@ +"""`video chain --out-file` (#1293) and `--resume-from` link numbering (#1300). + +The REAL ``run_chain`` and the REAL ``ChainLinkRecorder`` run, on the per-test database +the autouse ``_isolate_settings`` fixture provides. Only the transport +(``generate_video``), the frame extractor and the videos catalog are faked, so every +consumer of a clip path is the real one: the chain table, the next link's seed and +``--json``. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from typing import Any +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +import structlog + +from gflow_cli._cli_helpers import _OUT_FILE_DEPRECATION_WARNING +from gflow_cli.api.video import GenerateVideoRequest, Mode, VideoResult, VideoStatus +from gflow_cli.cli_video import video +from tests.cli.test_out_file_deprecation import CliRunner + +_KEPT_EVENT = "chain_clip_kept_at_download_path" + + +@pytest.fixture(autouse=True) +def _capture(install_log_capture: structlog.testing.LogCapture) -> structlog.testing.LogCapture: + """Keep structlog off stdout so ``--json`` stays one parseable document.""" + return install_log_capture + + +class _Flow: + """A fake Flow: one clip per successful link, a 403 for the prompts in ``fail``.""" + + def __init__(self, tmp_path: Path) -> None: + self.download_dir = tmp_path / "dl" + self.download_dir.mkdir(exist_ok=True) + self.fail: set[str] = set() + self.calls: list[tuple[str, Mode, str | None, bool]] = [] # prompt, mode, seed, ok + + async def generate_video(self, *, req: GenerateVideoRequest, **_: Any) -> VideoResult: + from gflow_cli.errors import WafRejectionError + + seed = req.start_image.read_text() if req.start_image is not None else None + ok = req.prompt not in self.fail + self.calls.append((req.prompt, req.mode, seed, ok)) + if not ok: + raise WafRejectionError("403") + media_id = "m" + req.prompt.removeprefix("link ") + clip = self.download_dir / f"{media_id}.mp4" + clip.write_bytes(b"\x00\x00\x00\x18ftypmp42") + return VideoResult( + status=VideoStatus(media_id=media_id, status="MEDIA_GENERATION_STATUS_SUCCESSFUL"), + local_path=clip, + ) + + +def _extract(src: Path, dst: Path, *, offset_ms: int = 0) -> Path: + assert src.exists(), f"seed extraction read a clip that is not on disk: {src}" + dst.write_text(src.name) # the frame names the clip it came from + return dst + + +def _invoke(tmp_path: Path, flow: _Flow, *, links: int, args: list[str]) -> tuple[Any, MagicMock]: + """Run `gflow video chain` once over a ``links``-link manifest.""" + from gflow_cli.chain import run_chain as real_run_chain + + manifest = tmp_path / "chain.jsonl" + manifest.write_text( + "\n".join(f'{{"prompt": "link {i}"}}' for i in range(links)), encoding="utf-8" + ) + client = MagicMock() + client.__aenter__ = AsyncMock(return_value=client) + client.__aexit__ = AsyncMock(return_value=None) + client.generate_video = AsyncMock(side_effect=flow.generate_video) + catalog = MagicMock() + assert real_run_chain.__kwdefaults__ is not None + with ( + patch("gflow_cli.cli_video._resolve_profile", return_value="default"), + patch("gflow_cli.cli_video._make_provider_dir", return_value=tmp_path / "prof"), + patch("gflow_cli.cli_video.OperationRecorder.open", return_value=catalog), + patch("gflow_cli.cli_video.FlowApiClient", return_value=client) as client_cls, + patch.dict(real_run_chain.__kwdefaults__, {"extractor": _extract}), + ): + result = CliRunner().invoke( + video, + ["chain", str(manifest), "--yes", "--json", "--out-dir", str(flow.download_dir)] + args, + ) + catalog.flow_client_cls = client_cls # lets a test see whether a browser was opened + return result, catalog + + +def _rows(tmp_path: Path, chain_id: str) -> list[tuple[int, Path]]: + from gflow_cli.config import get_settings + from gflow_cli.data.chain_repo import ChainLinkRecorder + + recorder = ChainLinkRecorder.open( + get_settings(), profile_name="default", profile_dir=tmp_path / "prof", chain_id=chain_id + ) + try: + return [(r.link_index, Path(r.local_path)) for r in recorder.completed_links()] + finally: + recorder.close() + + +def _catalog_paths(catalog: MagicMock) -> list[Path]: + return [c.kwargs["result"].local_path for c in catalog.record_completed_video.call_args_list] + + +@pytest.mark.parametrize("flag", ["--out-file", "-o"]) +def test_chain_out_file_writes_each_link_to_a_numbered_path(tmp_path: Path, flag: str) -> None: + """Two links with ``--out-file out/clip.mp4`` land at clip_1.mp4 and clip_2.mp4.""" + flow = _Flow(tmp_path) + out = tmp_path / "out" + result, catalog = _invoke(tmp_path, flow, links=2, args=[flag, str(out / "clip.mp4")]) + + assert result.exit_code == 0, result.output + expected = [out / "clip_1.mp4", out / "clip_2.mp4"] + assert all(p.is_file() for p in expected), list(out.iterdir()) + payload = json.loads(result.stdout) + assert [Path(link["local_path"]) for link in payload["links"]] == expected + assert _rows(tmp_path, payload["chain_id"]) == list(enumerate(expected)) + assert _catalog_paths(catalog) == expected + assert flow.calls[1][2] == "clip_1.mp4" # link 2 was seeded from the moved clip + assert "does not apply --out-file" not in result.stderr + if flag == "-o": + assert result.stderr.strip() == _OUT_FILE_DEPRECATION_WARNING + else: + assert _OUT_FILE_DEPRECATION_WARNING not in result.stderr + + +@pytest.mark.sharpit(allow_n_plus_one=True) # three runs of one chain, on purpose (#1300) +def test_resumes_keep_manifest_indexes_seed_i2v_and_bill_each_link_once(tmp_path: Path) -> None: + """#1300: a 3-link chain stops at link 1, then at link 2, then completes.""" + flow = _Flow(tmp_path) + out = tmp_path / "out" + clips = [out / f"clip_{n}.mp4" for n in (1, 2, 3)] + + flow.fail = {"link 1"} + first, _ = _invoke(tmp_path, flow, links=3, args=["--out-file", str(out / "clip.mp4")]) + assert first.exit_code == 21, first.output + chain_id = json.loads(first.stdout)["chain_id"] + assert _rows(tmp_path, chain_id) == [(0, clips[0])] + + flow.fail = {"link 2"} + resume = ["--resume-from", chain_id, "--out-file", str(out / "clip.mp4")] + second, _ = _invoke(tmp_path, flow, links=3, args=resume) + assert second.exit_code == 21, second.output + assert _rows(tmp_path, chain_id) == [(0, clips[0]), (1, clips[1])] + + flow.fail = set() + third, _ = _invoke(tmp_path, flow, links=3, args=resume) + assert third.exit_code == 0, third.output + assert [link["index"] for link in json.loads(third.stdout)["links"]] == [2] + assert _rows(tmp_path, chain_id) == list(enumerate(clips)) + assert all(p.is_file() for p in clips) + + assert flow.calls == [ + ("link 0", Mode.T2V, None, True), + ("link 1", Mode.I2V, "clip_1.mp4", False), + ("link 1", Mode.I2V, "clip_1.mp4", True), # resumed: i2v, seeded from link 1's clip + ("link 2", Mode.I2V, "clip_2.mp4", False), + ("link 2", Mode.I2V, "clip_2.mp4", True), + ] + billed = [prompt for prompt, _, _, ok in flow.calls if ok] + assert billed == ["link 0", "link 1", "link 2"] # no link generated twice + + +def test_a_resume_whose_seed_clip_is_gone_is_refused_before_submit(tmp_path: Path) -> None: + """#1300: with the last paid clip missing, the next link cannot be seeded; never t2v.""" + from gflow_cli.chain import ChainLinkResult + from gflow_cli.config import get_settings + from gflow_cli.data.chain_repo import ChainLinkRecorder + + missing = tmp_path / "dl" / "m0.mp4" + recorder = ChainLinkRecorder.open( + get_settings(), profile_name="default", profile_dir=tmp_path / "prof", chain_id="c1" + ) + recorder.record_chain_link( + ChainLinkResult(index=0, prompt="link 0", local_path=missing, media_id="m0") + ) + recorder.close() + flow = _Flow(tmp_path) + + result, catalog = _invoke(tmp_path, flow, links=2, args=["--resume-from", "c1"]) + + assert result.exit_code == 20, result.output + assert str(missing) in json.loads(result.stdout)["error"]["detail"] + assert flow.calls == [] + catalog.flow_client_cls.assert_not_called() + + +def test_an_existing_target_is_never_overwritten( + tmp_path: Path, _capture: structlog.testing.LogCapture +) -> None: + """A second run with the same --out-file keeps the old files and its own clips.""" + flow = _Flow(tmp_path) + out = tmp_path / "out" + out.mkdir() + (out / "clip_1.mp4").write_bytes(b"earlier run") + (out / "clip_2.mp4").mkdir() + + result, catalog = _invoke(tmp_path, flow, links=2, args=["--out-file", str(out / "clip.mp4")]) + + assert result.exit_code == 0, result.output + assert (out / "clip_1.mp4").read_bytes() == b"earlier run" + kept = [flow.download_dir / "m0.mp4", flow.download_dir / "m1.mp4"] + assert all(p.is_file() for p in kept) + payload = json.loads(result.stdout) + assert [Path(link["local_path"]) for link in payload["links"]] == kept + assert _rows(tmp_path, payload["chain_id"]) == list(enumerate(kept)) + assert _catalog_paths(catalog) == kept + warnings = [e for e in _capture.entries if e["event"] == _KEPT_EVENT] + assert [(w["link"], w["media_id"], w["kept_at"]) for w in warnings] == [ + (1, "m0", str(kept[0])), + (2, "m1", str(kept[1])), + ] + + +def test_a_failed_move_keeps_the_clip_and_removes_its_partial_copy( + tmp_path: Path, _capture: structlog.testing.LogCapture +) -> None: + """A move that dies mid-copy leaves no half file, and the paid clip stays recorded.""" + import shutil + + real_move = shutil.move + calls = 0 + + def _move(src: Any, dst: Any) -> Any: + nonlocal calls + calls += 1 + if calls == 2: + Path(dst).write_bytes(b"half") + raise OSError(28, "No space left on device") + return real_move(src, dst) + + flow = _Flow(tmp_path) + out = tmp_path / "out" + with patch("shutil.move", _move): + result, catalog = _invoke( + tmp_path, flow, links=2, args=["--out-file", str(out / "clip.mp4")] + ) + + assert result.exit_code == 0, result.output + assert not (out / "clip_2.mp4").exists() + expected = [out / "clip_1.mp4", flow.download_dir / "m1.mp4"] + payload = json.loads(result.stdout) + assert _rows(tmp_path, payload["chain_id"]) == list(enumerate(expected)) + assert _catalog_paths(catalog) == expected + warnings = [e for e in _capture.entries if e["event"] == _KEPT_EVENT] + assert [(w["link"], w["media_id"], w["kept_at"]) for w in warnings] == [ + (2, "m1", str(expected[1])) + ] + + +@pytest.mark.parametrize("gate", ["--yes", "--dry-run"]) +def test_an_out_file_under_a_file_is_a_usage_error_before_anything_runs( + tmp_path: Path, gate: str +) -> None: + blocker = tmp_path / "not_a_dir" + blocker.write_text("x") + manifest = tmp_path / "chain.jsonl" + manifest.write_text('{"prompt": "a"}\n{"prompt": "b"}\n', encoding="utf-8") + + with ( + patch("gflow_cli.cli_video._resolve_profile", return_value="default"), + patch("gflow_cli.cli_video._make_provider_dir", return_value=tmp_path / "prof"), + patch("gflow_cli.cli_video.FlowApiClient") as client_cls, + ): + result = CliRunner().invoke( + video, ["chain", str(manifest), gate, "--out-file", str(blocker / "clip.mp4")] + ) + + assert result.exit_code == 2, result.output + assert "--out-file" in result.stderr + assert str(blocker) in result.stderr + client_cls.assert_not_called() diff --git a/tests/cli/test_out_file_deprecation.py b/tests/cli/test_out_file_deprecation.py index ed46f08aa..a010ad180 100644 --- a/tests/cli/test_out_file_deprecation.py +++ b/tests/cli/test_out_file_deprecation.py @@ -355,24 +355,5 @@ def test_video_upscale_out_deprecation(tmp_path: Path) -> None: assert out_path.is_relative_to(dir2), out_path # -o DIR reached out_dir -@pytest.mark.parametrize("flag", ["--out-file", "-o"]) -def test_video_chain_says_the_file_path_is_not_applied(tmp_path: Path, flag: str) -> None: - """#1293: chain never wrote to the given path; it must say so instead of staying silent.""" - manifest = tmp_path / "chain.jsonl" - manifest.write_text('{"prompt": "a"}\n{"prompt": "b"}\n', encoding="utf-8") - - async def _ran(**_kwargs: object) -> None: - return None - - with ( - patch("gflow_cli.cli_video._resolve_profile", return_value="default"), - patch("gflow_cli.cli_video._make_provider_dir", return_value=tmp_path), - patch("gflow_cli.cli_video._run_chain", _ran), - ): - result = CliRunner().invoke( - main, ["video", "chain", str(manifest), "--yes", flag, str(tmp_path / "c.mp4")] - ) - - assert result.exit_code == 0, result.output - assert "video chain does not apply --out-file yet" in result.stderr - assert "#1293" in result.stderr +# `video chain --out-file` / `-o` (#1293) is pinned by tests/cli/test_cli_video_chain_out_file.py, +# which runs the real run_chain so the written files can be checked. diff --git a/tests/cli/test_predictable_output.py b/tests/cli/test_predictable_output.py index d0eecf57a..9462e436d 100644 --- a/tests/cli/test_predictable_output.py +++ b/tests/cli/test_predictable_output.py @@ -272,7 +272,7 @@ def _mock_generate_video(*args: object, **kwargs: object) -> VideoResult: def test_relocate_video_output_multi_count(self, tmp_path: Path) -> None: from gflow_cli.api.video import VideoResult, VideoStatus - from gflow_cli.cli_video import _relocate_video_output + from gflow_cli.paths import relocate_video_output v1_temp = tmp_path / "temp_1.mp4" v2_temp = tmp_path / "temp_2.mp4" @@ -289,7 +289,7 @@ def test_relocate_video_output_multi_count(self, tmp_path: Path) -> None: ) out_file = tmp_path / "multi_video" / "output.mp4" - relocated = _relocate_video_output([res1, res2], out_file) + relocated = relocate_video_output([res1, res2], out_file) assert len(relocated) == 2 assert relocated[0].local_path == tmp_path / "multi_video" / "output_1.mp4" @@ -305,7 +305,7 @@ def test_relocate_video_output_across_filesystems( import os from gflow_cli.api.video import VideoResult, VideoStatus - from gflow_cli.cli_video import _relocate_video_output + from gflow_cli.paths import relocate_video_output def cross_device(src: object, dst: object, *a: object, **k: object) -> None: raise OSError(errno.EXDEV, "Invalid cross-device link") @@ -322,7 +322,7 @@ def cross_device(src: object, dst: object, *a: object, **k: object) -> None: ) out_file = tmp_path / "other_mount" / "out.mp4" - relocated = _relocate_video_output(res, out_file) + relocated = relocate_video_output(res, out_file) assert relocated.local_path == out_file assert out_file.read_bytes() == b"clip" diff --git a/tests/e2e/test_chain_e2e.py b/tests/e2e/test_chain_e2e.py index 3ad18a4ca..d66f609b5 100644 --- a/tests/e2e/test_chain_e2e.py +++ b/tests/e2e/test_chain_e2e.py @@ -195,3 +195,74 @@ async def test_chain_two_link_seeds_i2v_from_last_frame( f"its start frame through the media dialog (i2v routed to ...StartImage, " f"not ...Text — issue #125); captured events: {events}" ) + + +def test_chain_cli_out_file_writes_numbered_mp4s_and_seeds_link_2( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """CHAIN-E2E-2 (#1293, #1300): the CLI, not ``run_chain``, honours ``--out-file``. + + ``gflow video chain <2-link manifest> --out-file /out/clip.mp4 --yes --json`` + writes ``clip_1.mp4`` and ``clip_2.mp4`` as mp4 files, ``--json`` reports exactly + those paths, and the catalog recorded link 2 as an i2v (a misrouted seeded link + aborts the chain with ``WireFormatError``, so exit 0 is itself route evidence). + + Spends ~2 Veo credits per run. + """ + import json + import sqlite3 + + from click.testing import CliRunner + + from gflow_cli.cli import main + from gflow_cli.config import reset_settings + from gflow_cli.paths import looks_like_video + + db = tmp_path / "e2e_chain_cli.db" + monkeypatch.delenv("GFLOW_CLI_HOME", raising=False) + monkeypatch.setenv("GFLOW_CLI_DB_PATH", str(db)) + reset_settings() + + name = os.environ.get("GFLOW_CLI_E2E_PROFILE", "").strip() + if not name: + pytest.skip("set GFLOW_CLI_E2E_PROFILE to a logged-in profile, then run with -m e2e_video") + from gflow_cli.auth import profile_dir as _resolve_profile_dir + + if not _resolve_profile_dir(name).exists(): + pytest.skip(f"profile not found for {name!r} — run `gflow auth login --profile {name}`") + + _model() # skips on an alias chains refuse + model_alias = os.environ.get(_E2E_MODEL_ENV, "veo-lite").strip().lower() + ratio = "9:16" if _aspect() is Aspect.PORTRAIT else "16:9" + manifest = tmp_path / "chain.jsonl" + manifest.write_text( + "\n".join(json.dumps({"prompt": p}) for p in (_LINK0_PROMPT, _LINK1_PROMPT)), + encoding="utf-8", + ) + out = tmp_path / "out" / "clip.mp4" + + result = CliRunner().invoke( + main, + [ + "video", "chain", str(manifest), + "--profile", name, "--model", model_alias, "--aspect", ratio, + "--out-dir", str(tmp_path / "dl"), "--out-file", str(out), + "--yes", "--json", + ], + ) # fmt: skip + + assert result.exit_code == 0, result.output + payload = json.loads(result.stdout[result.stdout.index("{\n") :]) + expected = [out.parent / "clip_1.mp4", out.parent / "clip_2.mp4"] + assert [Path(link["local_path"]) for link in payload["links"]] == expected + assert [Path(p) for p in payload["completed_paths"]] == expected + for clip in expected: + assert clip.is_file(), f"--out-file clip missing: {clip}" + assert looks_like_video(clip.read_bytes()[:16]), f"not an mp4: {clip}" + + # The database holds this run only; chain links record command "video t2v"/"video i2v". + with sqlite3.connect(db) as conn: + modes = [ + row[0] for row in conn.execute("SELECT mode FROM operations ORDER BY started_at, rowid") + ] + assert modes == ["t2v", "i2v"], f"link 2 must go out as i2v; catalog modes: {modes}" diff --git a/tests/features/test_video_chain_steps.py b/tests/features/test_video_chain_steps.py index 501777620..da077ff43 100644 --- a/tests/features/test_video_chain_steps.py +++ b/tests/features/test_video_chain_steps.py @@ -151,13 +151,20 @@ async def _boom(**_kwargs: Any) -> Any: chain_state["run_chain"] = mock_run +def _done_link(tmp_path: Path, index: int) -> MagicMock: + """A recorded link whose clip is on disk, so a resume can seed from it (#1300).""" + clip = tmp_path / f"done{index}.mp4" + clip.touch() + return MagicMock(link_index=index, local_path=str(clip)) + + @given(parsers.parse("the recorder reports {n:d} completed link")) @given(parsers.parse("the recorder reports {n:d} completed links")) def _recorder_completed( - chain_state: dict[str, Any], monkeypatch: pytest.MonkeyPatch, n: int + chain_state: dict[str, Any], monkeypatch: pytest.MonkeyPatch, tmp_path: Path, n: int ) -> None: recorder = MagicMock(name="ChainLinkRecorder") - recorder.completed_links.return_value = [MagicMock() for _ in range(n)] + recorder.completed_links.return_value = [_done_link(tmp_path, i) for i in range(n)] monkeypatch.setattr( "gflow_cli.data.chain_repo.ChainLinkRecorder.open", lambda *_a, **_kw: recorder ) @@ -167,16 +174,17 @@ def _recorder_completed( @given(parsers.parse("the recorder reports {n:d} completed link whose seed frame is absent")) def _recorder_completed_no_seed( - chain_state: dict[str, Any], monkeypatch: pytest.MonkeyPatch, n: int + chain_state: dict[str, Any], monkeypatch: pytest.MonkeyPatch, tmp_path: Path, n: int ) -> None: """Models the record-before-extract guarantee: the link's clip is recorded (so ``completed_links`` counts it and resume skips regeneration) even though its seed frame file is absent. Resume must therefore re-submit only the remaining links — the recorded link is NOT regenerated.""" recorder = MagicMock(name="ChainLinkRecorder") - completed_link = MagicMock() - completed_link.seed_frame_path = None # extraction never completed - recorder.completed_links.return_value = [completed_link for _ in range(n)] + completed = [_done_link(tmp_path, i) for i in range(n)] + for link in completed: + link.seed_frame_path = None # extraction never completed + recorder.completed_links.return_value = completed monkeypatch.setattr( "gflow_cli.data.chain_repo.ChainLinkRecorder.open", lambda *_a, **_kw: recorder ) diff --git a/tests/test_chain.py b/tests/test_chain.py index cedd324e2..a54ad8bfc 100644 --- a/tests/test_chain.py +++ b/tests/test_chain.py @@ -511,3 +511,81 @@ async def _gen(*, req: GenerateVideoRequest, **_: Any) -> VideoResult: failed_req, failed_exc = failed[0] assert failed_req.prompt == "the cat stretches and walks off" assert isinstance(failed_exc, WireFormatError) + + +async def test_a_resumed_link_keeps_its_manifest_index_and_is_seeded(tmp_path: Path) -> None: + """#1300: link 2 of a resumed chain is link 2, an i2v seeded from link 1's clip.""" + prev_clip = tmp_path / "link1.mp4" + prev_clip.write_bytes(b"\x00\x00\x00\x18ftypmp42") + client = _make_client([_ok_result("m2", tmp_path / "link2.mp4")]) + recorder = MagicMock() + extracted: list[tuple[Path, Path]] = [] + + def _extract(src: Path, dst: Path, *, offset_ms: int = 0) -> Path: + dst.write_bytes(b"\xff\xd8\xff\xd9") + extracted.append((src, dst)) + return dst + + out = await run_chain( + client=client, + links=[ChainLinkSpec(prompt="p2")], + out_dir=tmp_path, + model=VideoModel.VEO_3_1_LITE, + extractor=_extract, + recorder=recorder, + start_index=2, + seed_clip=prev_clip, + ) + + req = client.generate_video.await_args.kwargs["req"] + assert req.mode == Mode.I2V + assert extracted == [(prev_clip, tmp_path / "link1_lastframe.jpg")] + assert req.start_image == tmp_path / "link1_lastframe.jpg" + assert [r.index for r in out] == [2] + assert recorder.record_chain_link.call_args.args[0].index == 2 + assert out[0].frame_path is None # still the final link + + +async def test_a_resumed_link_without_a_seed_is_refused_not_sent_as_t2v(tmp_path: Path) -> None: + """#1300: an index past 0 with nothing to seed it must not fall back to t2v.""" + client = _make_client([_ok_result("m1", tmp_path / "link1.mp4")]) + + with pytest.raises(ValueError, match="seed_clip"): + await run_chain( + client=client, + links=[ChainLinkSpec(prompt="p1")], + out_dir=tmp_path, + model=VideoModel.VEO_3_1_LITE, + extractor=_fake_extractor([]), + start_index=1, + ) + client.generate_video.assert_not_awaited() + + +async def test_a_raising_relocate_leaves_the_downloaded_clip_recorded(tmp_path: Path) -> None: + """#1293 review: the relocate hook runs off the loop, and any error it raises is + absorbed so the paid clip is still recorded and returned at its download path.""" + import threading + + clip = tmp_path / "link0.mp4" + client = _make_client([_ok_result("m0", clip)]) + recorder = MagicMock() + threads: list[int] = [] + + def _relocate(index: int, result: VideoResult) -> VideoResult: + threads.append(threading.get_ident()) + raise RuntimeError("relocate bug") + + out = await run_chain( + client=client, + links=[ChainLinkSpec(prompt="p0")], + out_dir=tmp_path, + model=VideoModel.VEO_3_1_LITE, + extractor=_fake_extractor([]), + recorder=recorder, + relocate=_relocate, + ) + + assert threads and threads[0] != threading.get_ident(), "relocate ran on the event loop" + assert [r.local_path for r in out] == [clip] + assert recorder.record_chain_link.call_args.args[0].local_path == clip diff --git a/website/docs/CONFIGURATION.md b/website/docs/CONFIGURATION.md index 512f35bfc..cd1bd6c2e 100644 --- a/website/docs/CONFIGURATION.md +++ b/website/docs/CONFIGURATION.md @@ -639,11 +639,12 @@ waits for each link in turn, so total wallclock is the sum of all link waits. | `--max-links N` | unset | Cap link count; exit 11 (`ConfigurationError`) if the manifest has more. A spend guardrail. | | `-y` / `--yes` | off | Skip the pending video operation confirmation prompt. | | `--dry-run` | off | Print the pending video operation plan and submit nothing. | -| `--resume-from CHAIN_ID` | unset | Resume a prior chain by its id; already-completed links are skipped (not regenerated). | +| `--resume-from CHAIN_ID` | unset | Resume a prior chain by its id; already-completed links are skipped (not regenerated), and the next link is an i2v seeded from the last completed clip, which must still be at its recorded path (exit 20 otherwise, before anything is billed; [#1300](https://github.com/ffroliva/gflow-cli/issues/1300)). | | `--jitter F` | `0.0` | Random `0..F` second pause **between** links (anti-bot cadence; never before link 0). | | `--seed-offset MS` | `0` | Extract the seed frame this many ms before EOF (fade-to-black guard). | | `--aspect` | `9:16` | `9:16` / `16:9`. Applied uniformly to every link (continuity requirement). | -| `--out-dir DIR` | output root | Directory for the link mp4s + `linkN_lastframe.jpg` seed frames. | +| `--out-file FILE` | unset | Link i's clip goes to `_` next to FILE (`clip.mp4` gives `clip_1.mp4`, `clip_2.mp4`), numbered over the whole manifest so `--resume-from` keeps each name; seed frames stay under `--out-dir`. An existing file or folder there is never replaced: that clip, like one whose move fails, stays where it downloaded, recorded there, with a warning. A FILE under an existing file is a usage error (exit 2) before anything runs. Deprecated spelling: `-o/--output` ([#1293](https://github.com/ffroliva/gflow-cli/issues/1293)). | +| `--out-dir DIR` | output root | Directory for the `linkN_lastframe.jpg` seed frames, and for the link mp4s unless `--out-file` is given. | | `--profile NAME` | default profile | Per-subcommand profile override. | | `--json` | off | Emit a machine-readable JSON result. | diff --git a/website/docs/USAGE.md b/website/docs/USAGE.md index f3aed195b..93a9cd460 100644 --- a/website/docs/USAGE.md +++ b/website/docs/USAGE.md @@ -735,8 +735,8 @@ All prompts in a batch share one Flow project. The editor is opened once and sta > instead of proceeding on Flow's sticky default (typically x2) and silently > changing what the run bills. > `t2v`, `i2v`, `r2v`, `video chain`, and `video extend` additionally take `--out-file PATH` (explicit file destination). -> `video chain` does not apply it yet: it says so on stderr and the clips land under `--out-dir` ([#1293](https://github.com/ffroliva/gflow-cli/issues/1293)). -> The mp4 lands at `` or `/.mp4`. +> `video chain` writes link i's clip to `_` next to PATH (`--out-file out/clip.mp4` gives `out/clip_1.mp4`, `out/clip_2.mp4`, …), numbered over the whole manifest even for one link, so `--resume-from` writes the remaining links under the names a full run would; seed frames stay under `--out-dir`, and `--json` and the catalog report the numbered paths. An existing file or folder at a numbered path is never replaced: that clip, like one whose move fails, stays at its `--out-dir` path, is recorded there, and a warning names it ([#1293](https://github.com/ffroliva/gflow-cli/issues/1293)). +> For `t2v`, `i2v` and `r2v` the mp4 lands at `` or `/.mp4`. > > **`i2v` model rules (issues #125 / #626):** every model — `omni-flash` > included — supports i2v with a **start frame** and with an **end frame** @@ -1191,14 +1191,19 @@ Options: --dry-run Resolve the manifest, print the plan + credit cost, and spend nothing. --resume-from CHAIN_ID Resume a prior chain by its id; already-paid links - are skipped (not re-billed). + are skipped (not re-billed), and the next link is + seeded from the last paid clip, which must still be + at its recorded path (exit 20 otherwise). --jitter FLOAT Random 0..JITTER second pause between links (anti-bot cadence). [default: 0.0] --seed-offset INTEGER Extract the seed frame this many ms before EOF (fade-to-black guard). [default: 0] --aspect [9:16|16:9] Uniform aspect for every link. [default: 9:16] --profile NAME Profile name (overrides default). - --out-dir DIR Directory for the link mp4s + seed frames. + --out-file FILE Write link i's clip to FILE's stem plus _i + (clip.mp4 -> clip_1.mp4, clip_2.mp4). + --out-dir DIR Directory for the seed frames, and for the link + mp4s unless --out-file is given. --json Emit a machine-readable JSON result. ``` @@ -1237,7 +1242,10 @@ the manifest is currently informational). ### Output — N clips, not one file Each link is saved as its own mp4 (plus a `linkN_lastframe.jpg` seed frame -between links) under `--out-dir` (or the default output dir). **Stitching the +between links) under `--out-dir` (or the default output dir). With +`--out-file out/clip.mp4` link i's clip is written to `out/clip_.mp4` +instead (1-based over the whole manifest, so a `--resume-from` run keeps each +link's number); the seed frames stay under `--out-dir`. **Stitching the clips into a single video is a separate step — use `gflow scene` to concatenate them server-side.** Chain does not auto-concat: see the *deferred auto-concat* entry in @@ -2372,7 +2380,7 @@ The table below is the default (legacy) set. `GFLOW_CLI_EXIT_CODES=2` opts in to | `17` | `ModelModeIncompatibilityError` | The chosen video model can't do the requested mode — today that is `omni-flash` or `omni-1.2-flash` for `chain` (issues #125, #626) | Use a Veo 3.1 model (`veo-lite` / `veo-fast` / `veo-quality` / `veo-lite-lp`) for `chain`. Single-clip `i2v` with omni-flash, `--end-frame` included, is accepted | | `18` | `VideoModelSelectionError` | gflow could not select the requested video model in Flow's editor for an `i2v` run (model-picker option not found) | Usually transient — retry; if it persists, Flow's model-picker UI changed (report referencing #125) | | `19` | `SceneConcatError` | Server-side scene render/concat failed (`gflow scene --out-file`) | Retry; the recorded compose survives, so re-render is safe | -| `20` | `FrameExtractionError` | Could not extract the last frame for a video chain link | Check the source video downloaded intact; retry the link | +| `20` | `FrameExtractionError` | Could not extract the last frame for a video chain link, or a `video chain --resume-from` whose last completed clip is no longer at its recorded path (refused before anything is billed, [#1300](https://github.com/ffroliva/gflow-cli/issues/1300)) | Check the source video downloaded intact; retry the link. For a resume, put the clip back at the path named, or start a new chain | | `21` | `ChainPartialError` | A video chain stopped mid-way; earlier links completed | Resume from the last completed link shown in the error | | `22` | `UpscaleUnavailableError` | 4K image upscale is gated to Flow **Ultra** accounts; or requested video export quality is disabled | For images, use `--scale 2k` or upgrade the Flow plan; for videos, check available options in Flow | | `23` | `UiSelectorDriftError` | A Flow editor control could not be located — Google changed the frontend (issues #183, #493) — **or** a blocking announcement overlay survived dismissal, so no control below it can be clicked (probe `overlay_close_button`, #593). On the migrated host a missing submit control is checked against the wallet first and reported as **37** when Flow swapped in its insufficient-credits warning, so a short balance no longer arrives here | If the detail says the model picker is **hidden**, nothing was submitted: turn the **Agent** chip off in a browser and re-run ([#788](https://github.com/ffroliva/gflow-cli/issues/788)). Otherwise update gflow-cli; file a bug with the probe name + the diagnostics JSON / debug screenshot referenced in the error message, plus the incident bundle's `report.md` when one was written. For `overlay_close_button`: open the project once in Chrome and dismiss the announcement — the dismissal persists on your account |