Repository navigation
fix(data): refuse an image media id instead of blaming the project, the trash and your prompt (#877) - #878
Merged
Conversation
…he trash and your prompt (#877) `gflow data download` recovers 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 — so an image id could never work. It was accepted anyway. The command opened Chrome, waited 45 s, and produced three guesses for a condition the catalog row states outright: wrong project, clip moved to trash, and — inherited from WireFormatError's class default — "retry with a simpler prompt text", on a command that has no prompt. Measured live on 2026-09-18 against an image generated twelve minutes earlier, in that project, not trashed, catalog row intact. None of the three applied. `download_media` now reads `asset.kind` before anything else and exits 11. `record_local_file` hardcoding `media_type="video/mp4"` was the other sign this path was always video-only. The timeout that remains, for a video Flow genuinely does not report, drops the generation-payload remediation and says what to actually check. Surfaces that claimed more than the code delivers, all corrected: the CLI help, the `gflow_download_media` MCP description, and docs/USAGE.md each said "asset". A note on the fix's own first version, because it is the same bug one level up: it passed every test and was still wrong. The detail was right, but ConfigurationError's class-default remediation took over and told the user to "check that the transport name is registered via make_transport()". One misleading hint swapped for another, green the whole way, because the tests read the detail and not the hint. A live run caught it. The test now asserts the remediation, and carries that story. The MCP twin gets its own test rather than trusting the shared service: it does not re-raise, it funnels through `_guarded` into a problem-details envelope, so what needed pinning is that the reason survives the conversion — an agent handed a generic failure would retry an id that can never work.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe download path now rejects image media IDs before browser or network activity. Video recovery continues through signed URLs, with updated clip-specific errors and video-only CLI, MCP, and documentation text. ChangesVideo media recovery
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to Image downloads now fail immediately with the documented video-only error, while video recovery behavior remains available. No actionable merge risk is identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 5 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
gflow data downloadrecovers video only. The signed URL it needs comes from theas29srecord Flow emits when a clip's own route loads, and an image's route does not carry one — so an image media id could never work.It was accepted anyway: the command opened Chrome, waited 45 s, and produced three guesses for a condition the catalog row states outright.
Measured live 2026-09-18 against an image generated twelve minutes earlier, in that project, not trashed, catalog row intact. Wrong project — no. Trashed — no. Simpler prompt — there is no prompt; it is a download command.
The fix
download_mediareadsasset.kindbefore anything else and exits 11:No browser, no wait.
record_local_filehardcodingmedia_type="video/mp4"was the other sign this path was always video-only.The timeout that remains — for a video Flow genuinely does not report — drops the inherited generation-payload remediation and describes what to actually check.
Surfaces corrected. The CLI help, the
gflow_download_mediaMCP description anddocs/USAGE.mdeach said "asset". All three now say video and name the refusal.The part worth reading
The first version of this fix passed every test and was still wrong.
The detail was right, but
ConfigurationError's class-default remediation took over:One misleading hint swapped for another — green the whole way, because the tests asserted the detail and not the hint. A live run caught it.
That is the same defect the fix is about, one level up, and it is now the fourth instance this session: a class-default remediation written for one situation, inherited by a raise site in another (#803, #875, #877, and this). The test now asserts the remediation and carries the story in a comment.
Tests
test_refuses_an_image_before_launching_a_browser— asserts the refusal, that no browser is launched, that the old guesses are gone, and that the remediation is not the transport-registry default.test_the_mcp_twin_refuses_an_image_too— its own test, not inherited trust. The twin does not re-raise:_guardedfunnels aGFlowErrorinto a problem-details envelope, so what needed pinning is that the reason survives the conversion. An agent handed a masked generic failure would retry an id that can never work.254 recovery + MCP tests pass.
Verification
Live, on a profile Flow serves from
flow.google.com, $0:Gates:
ruff check·ruff format· repo hygiene (1170 files) · doc links (197 files) · website mirror regenerated and in sync · website PII ·pyright src0 errors.No Flow surface behaviour changed — the refusal happens before any transport call — so no new e2e is owed. The existing
tests/e2e/test_data_download_e2e.pycovers the video path it does not touch.Closes #877
Summary by CodeRabbit
New Behavior
Documentation