diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b1a9348b..b8f4ca8b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **`gflow data download` refuses an image immediately instead of blaming the project, + the trash and your prompt (#877).** Recovery is video-only: the signed URL comes from + the `as29s` record Flow emits when a clip's own route loads, and an image's route does + not carry one. The command accepted an image media id anyway, launched Chrome, waited + 45 s, and then produced three guesses — wrong project, clip in trash, *"retry with a + simpler prompt text"* — for a condition the catalog row states outright, on a command + that has no prompt. It now checks `kind` before anything else and exits 11 saying + recovery is video-only. The CLI help, the `gflow_download_media` MCP description and + `docs/USAGE.md` all said "asset"; they now say video. The timeout message that remains + for a genuine video miss drops the inherited generation-payload remediation and + describes what to actually check. + ### Added - **`gflow data download ` — recover a billed asset whose download failed.** diff --git a/docs/USAGE.md b/docs/USAGE.md index 2e7e7477d..dca33fe69 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -1329,9 +1329,14 @@ Options: ## `gflow data download` -Fetch an already-generated asset from Flow by its media ID, for the case where the +Fetch an already-generated **video** from Flow by its media ID, for the case where the generation succeeded but its download did not. +> **Video only.** The signed URL this needs comes from a record Flow emits when a clip's +> own route loads, and an image's route does not carry one. An image media ID is refused +> immediately with exit 11 and a message saying so — it does not open a browser or wait. +> Tracked in [#877](https://github.com/ffroliva/gflow-cli/issues/877). + That case is real and costs money: on `flow.google.com` a generation whose signed media URL is not observed within the grace window exits 7 (`WireFormatError`) **after the credit has been spent**. The clip is in the Flow project, the catalog has a row for it, diff --git a/src/gflow_cli/api/transports/migrated_recover.py b/src/gflow_cli/api/transports/migrated_recover.py index fd39e7de9..c60c8b096 100644 --- a/src/gflow_cli/api/transports/migrated_recover.py +++ b/src/gflow_cli/api/transports/migrated_recover.py @@ -104,11 +104,22 @@ async def on_response(response: Any) -> None: page.remove_listener("response", on_response) if not found: + # #877: the class-default remediation on WireFormatError is written for + # generation payloads — "check request payload parameters or retry with a + # simpler prompt text" — and this command has neither a payload nor a prompt. + # An image id no longer reaches here (the service refuses on the catalog row), + # so the remaining causes really are about the clip. raise WireFormatError( detail=( f"migrated host: no signed media URL for {media_id} within " - f"{wait_s:.0f}s of opening its clip route. The media id may belong to " - "another project, or the clip may have been moved to trash in Flow." + f"{wait_s:.0f}s of opening its clip route." + ), + remediation_hint=( + "Flow did not report a media URL for this clip. Open the project in " + "Flow and check the clip is still there — a clip moved to trash, or a " + "media id belonging to a different project, both look like this. If it " + "is visible and playable, re-run once: the record is fetched as the " + "route loads and a slow load can miss the window." ), route=_ROUTE, ) diff --git a/src/gflow_cli/cli_data.py b/src/gflow_cli/cli_data.py index 1d14243ff..833c96542 100644 --- a/src/gflow_cli/cli_data.py +++ b/src/gflow_cli/cli_data.py @@ -348,7 +348,7 @@ async def _run_media(*, profile: str | None, media_id: str) -> None: ) @click.option("--json", "as_json", is_flag=True, help="Emit a JSON summary instead of text.") def download(media_id: str, out_dir: Path | None, profile: str | None, as_json: bool) -> None: - """Fetch an already-generated asset from Flow by its media ID. + """Fetch an already-generated video from Flow by its media ID. For a generation that finished and was billed but whose download failed — the migrated host's 20s URL grace expiring leaves exactly this state, with the clip in @@ -358,6 +358,10 @@ def download(media_id: str, out_dir: Path | None, profile: str | None, as_json: Opens the clip's own route, takes the signed URL Flow reports for it, and verifies the bytes against the size Flow records before writing. Also writes the `local_files` row, so `gflow data list videos` stops showing `copy_count: 0`. + + Video only. The signed URL comes from a record Flow emits when a clip's own route + loads; an image's route does not carry one, so an image media ID is refused + immediately with exit 11 rather than opening a browser. See issue #877. """ run_with_handlers( lambda: _run_download(media_id=media_id, out_dir=out_dir, profile=profile, as_json=as_json), diff --git a/src/gflow_cli/mcp/tools.py b/src/gflow_cli/mcp/tools.py index f963ac003..bf9720490 100644 --- a/src/gflow_cli/mcp/tools.py +++ b/src/gflow_cli/mcp/tools.py @@ -1436,11 +1436,13 @@ async def gflow_character_show( @server.tool( name="gflow_download_media", description=( - "Fetch an already-generated asset from Flow by its media ID and write it to " + "Fetch an already-generated VIDEO from Flow by its media ID and write it to " "disk. For a generation that finished and was billed but whose download failed " - "— the asset is in the Flow project and the local catalog shows no file for it. " + "— the clip is in the Flow project and the local catalog shows no file for it. " "Spends no credits: the generation was already paid for. The bytes are verified " - "against the size Flow reports before the file is written." + "against the size Flow reports before the file is written. Video only: an image " + "media ID is refused immediately (exit 11) because the signed URL this needs " + "comes from a record only a clip's route emits. See issue #877." ), ) @_guarded diff --git a/src/gflow_cli/services/media_recovery.py b/src/gflow_cli/services/media_recovery.py index 09c8786cb..b7bc8adca 100644 --- a/src/gflow_cli/services/media_recovery.py +++ b/src/gflow_cli/services/media_recovery.py @@ -18,10 +18,10 @@ import structlog from gflow_cli.config import get_settings -from gflow_cli.data.models import AssetLookup, LocalFileRecord +from gflow_cli.data.models import AssetKind, AssetLookup, LocalFileRecord from gflow_cli.data.repository import DataRepository from gflow_cli.data.store import DataStore -from gflow_cli.errors import DataStoreError +from gflow_cli.errors import ConfigurationError, DataStoreError log = structlog.get_logger(__name__) @@ -115,6 +115,26 @@ async def download_media( from gflow_cli.api.transports.migrated_recover import recover_clip # noqa: PLC0415 asset = resolve_asset(media_id, profile=profile, route=_ROUTE) + if asset.kind is not AssetKind.VIDEO: + # #877: the signed URL comes from the `as29s` record a clip route emits, which + # is a video mechanism — an image's route never emits one. Accepting the id + # anyway cost 45 s in a browser and then produced three guesses (wrong project, + # trashed clip, "retry with a simpler prompt") for a condition this row states + # outright. Refuse here, on what the catalog already knows. + raise ConfigurationError( + detail=( + f"Media {media_id!r} is {asset.kind.value}, and `gflow data download` " + "recovers video only. The signed URL it needs comes from the record " + "Flow emits when a clip's own route loads, and an image's route does " + "not carry one. Tracking the image path in issue #877." + ), + remediation_hint=( + "Nothing to retry — this is a capability gap, not a fault. Open the " + "image in Flow and save it from there, or pass a video media id. " + "`gflow data list images` shows which rows are images." + ), + route=_ROUTE, + ) if not asset.flow_project_id: raise DataStoreError( detail=( diff --git a/tests/services/test_media_recovery.py b/tests/services/test_media_recovery.py index 7eef27e7c..e24013149 100644 --- a/tests/services/test_media_recovery.py +++ b/tests/services/test_media_recovery.py @@ -16,7 +16,7 @@ from gflow_cli.data.repository import DataRepository from gflow_cli.data.store import DataStore -from gflow_cli.errors import DataStoreError +from gflow_cli.errors import ConfigurationError, DataStoreError from gflow_cli.services.media_recovery import ( DownloadedMedia, download_media, @@ -136,6 +136,71 @@ def _boom(*_a: Any, **_k: Any) -> None: # pragma: no cover - must not run with pytest.raises(DataStoreError, match="no project id"): await download_media(media_id="vid-media-alice-0", profile="alice", out_dir=None) + @pytest.mark.asyncio + async def test_refuses_an_image_before_launching_a_browser( + self, catalog: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """#877: recovery is video-only, and the refusal has to be immediate. + + The signed URL comes from the ``as29s`` record a clip route emits; an image's + route never emits one, so the old code accepted the id, opened Chrome, waited + 45 s and then blamed the project, the trash and the user's prompt — three + guesses, all false, for a condition the catalog row states outright. + """ + + def _boom(*_a: Any, **_k: Any) -> None: # pragma: no cover - must not run + raise AssertionError("a browser was launched for an image media id") + + monkeypatch.setattr("gflow_cli._cli_helpers._make_provider_dir", _boom) + + with pytest.raises(ConfigurationError, match="video") as excinfo: + await download_media(media_id="img-media-alice-0-0", profile="alice", out_dir=None) + + detail = str(excinfo.value) + assert "877" in detail, detail + # Never repeat the old guesses: the row says it is an image. + assert "trash" not in detail.lower(), detail + + # The REMEDIATION, not just the detail. A live run caught this: the first + # version of the fix left ConfigurationError's class default in place, so the + # message said "check that the transport name is registered via + # make_transport()" — one misleading hint swapped for another, with every + # test still green because they only read the detail. + hint = excinfo.value.remediation_hint + assert "make_transport" not in hint, hint + assert "transport" not in hint.lower(), hint + assert "image" in hint.lower(), hint + + @pytest.mark.asyncio + async def test_the_mcp_twin_refuses_an_image_too( + self, catalog: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The MCP tool is a separate surface, so it gets its own assertion. + + It shares the service, and the service was never the risk — the adapter is. + Here the adapter deliberately does NOT re-raise: `_guarded` funnels a + `GFlowError` into a problem-details envelope. So the thing to pin is that the + reason survives that conversion — an agent that got a generic failure would + retry a media id that can never work. + """ + from gflow_cli.mcp import tools as mcp_tools + + def _boom(*_a: Any, **_k: Any) -> None: # pragma: no cover - must not run + raise AssertionError("the MCP twin launched a browser for an image") + + monkeypatch.setattr("gflow_cli._cli_helpers._make_provider_dir", _boom) + + result = await mcp_tools.gflow_download_media( + media_id="img-media-alice-0-0", profile="alice" + ) + + assert result["status"] == "error", result + blob = str(result) + assert "video only" in blob, blob + assert "877" in blob, blob + # Not the masked generic envelope — that would lose the reason entirely. + assert "unexpected" not in blob.lower(), blob + @pytest.mark.asyncio async def test_reports_the_written_file_and_records_it( self, catalog: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch diff --git a/website/docs/USAGE.md b/website/docs/USAGE.md index e098f9632..f8c7e446c 100644 --- a/website/docs/USAGE.md +++ b/website/docs/USAGE.md @@ -1329,9 +1329,14 @@ Options: ## `gflow data download` -Fetch an already-generated asset from Flow by its media ID, for the case where the +Fetch an already-generated **video** from Flow by its media ID, for the case where the generation succeeded but its download did not. +> **Video only.** The signed URL this needs comes from a record Flow emits when a clip's +> own route loads, and an image's route does not carry one. An image media ID is refused +> immediately with exit 11 and a message saying so — it does not open a browser or wait. +> Tracked in [#877](https://github.com/ffroliva/gflow-cli/issues/877). + That case is real and costs money: on `flow.google.com` a generation whose signed media URL is not observed within the grace window exits 7 (`WireFormatError`) **after the credit has been spent**. The clip is in the Flow project, the catalog has a row for it,