Validate video duration limits and audio parameter types

[christophervoelpel]

Summary

Evaluates resolution-dependent duration capability constraints independently of whether resolution is present in params (validating against the union of all allowed durations across the model's supported resolutions when omitted), and validates that generate_audio, when present, is a boolean.

Impact Characterization

An earlier review overstated the omission of resolution on generate_video submissions as a runaway Veo cost risk. The omission is NOT a cost vector: generate_video.execute raises TypeError immediately before any Veo generation call is dispatched. The actual harm is a fail-open contract violation plus wasted worker queue work — a request that should have been rejected at the front door instead travels to a worker and fails there. This change ensures front-door validation fails closed on invalid durations.

Design Decisions & Follow-up Note

• Resolution-omitted duration union check: When resolution is omitted, duration_seconds is checked against the union of allowed durations across all supported resolutions for the model. This provides a minimal fail-closed floor (rejecting durations like 9999 or non-integers). Outright rejection of omitted resolution was not applied because ui/definitions/actions.json does not currently express parameter requiredness.
• Required-parameter enforcement (SM-11, open follow-up): Enforcing required action parameters across all actions requires establishing a source of truth (e.g. deriving requiredness from Python execute() signatures or adding an explicit "required": true annotation in actions.json). This should be addressed in a separate PR with dedicated design discussion and deliberate test updates.
• Audio boolean check: Validates that generate_audio elements are booleans, rejecting non-boolean types with MALFORMED_SUBMISSION.

Verification

• TZ=UTC python -m pytest -q test/test_submission_validation.py: 161 passed on the unchanged code head a4314fe.
• This follow-up updates the title and scope description only. Required-parameter enforcement remains open as SM-11.

TAG=agy
CONV=1ece3a5d-2611-4c94-b69e-9e85c2753833

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [sam-bailey]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [RyanSibbaluca]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [nfrontini]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [Jelena0921]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [andersonfreitas]

[christophervoelpel]

Review: merge after a retitle

Multi-agent review (reviewer → independent critique agent re-verifying each claim against the code). The critique pass overturned two of the reviewer's refactor suggestions; details below.

The two checks this PR adds are correct, and the fail-closed change is safe to ship. The union-vs-pairwise distinction is well reasoned and the comment explains why it's a floor rather than a guarantee — the existing pairwise cross-product test still holds.

Important

The title claims work the body explicitly defers

Title: "Fail closed when required action parameters are absent."
Body: "Required-parameter enforcement … should be addressed in a separate PR."

Nothing in the diff enforces required parameters. Merging under this title closes SM-11 silently. Suggest retitling to what it does — e.g. "Validate duration when resolution is omitted; require boolean generate_audio" — and leaving SM-11 open.

Fail-closed risk assessment: benign, no announcement needed

I checked whether this rejects previously-accepted submissions, and it can't meaningfully:

• The union is taken over all resolutions, so it can only reject a duration that no resolution permits (veo union {4,6,8}, omni {3..10}).
• A resolution-omitted submission could never have succeeded anyway — generate_video.execute() declares the param with no default, so it TypeErrors at the worker before anything expensive happens.
• Falsy values are handled correctly: generate_audio: False still passes on veo (audio_always_on: False), and the isinstance(duration, bool) guard correctly stops True sneaking through as 1.
• workflow_examples/image2video.json is the only example with generate_audio, and it's a real true.

Worth fixing

1. Move the generate_audio bool check above the model loop. It's invoked inside for model in model_list: (submission_validation.py:~597) but never reads caps, so it re-runs per model and falsifies _capability_violation's own docstring (:162-175), which says "a parameter is only checked when it is present in params AND the model's capabilities carry the matching field". A 3-line move puts it with the shape checks; alternatively just amend the docstring.

2. Add one test for the deliberate union hole. models.json:57 has {"720p":[4,6,8],"1080p":[4,6,8],"4k":[8]}, so with resolution omitted the union branch intentionally accepts duration=4 for a 4k-capable model. That's the only genuinely new semantic in this PR and it's untested — worth more than the three tests below.

3. Error message omits the offending value (generate_audio must be a boolean), unlike every sibling message which interpolates {value!r}. Same for the union message: it should name the allowed durations.

Suggested deletions (~20 of 47 test lines)

These three pass against old code, so they guard nothing new:

• test_duration_over_cap_rejected_when_resolution_present — duplicates the existing test_veo_duration_not_allowed_at_4k / test_duration_not_allowed_for_resolution
• test_full_valid_submission_passes — covered by test_veo_duration_pairwise_cross_product_accepted and test_omni_full_valid_submission_accepted
• test_bool_generate_audio_passes

Keepers (these genuinely fail on old code): the two *_when_resolution_omitted tests and test_non_bool_generate_audio_rejected. Also _FULL_VALID_VIDEO restates _VALID_VIDEO (test_submission_validation.py:525) — write _FULL_VALID_VIDEO = {**_VALID_VIDEO, ...}.

Explicitly not recommended

• Don't drive the bool check off actions.json's declared "type": "bool". It looks like the obvious de-duplication, but _action_params() (line 105) returns a set of names, not the type dict — so this needs a new accessor plus a decision about where a generic type-checker lives, all for one boolean param. The hard-coded check is leaner; revisit at the second bool param.
• Don't merge the two duration branches. They're genuinely different (pairwise vs union), and collapsing them costs the per-resolution error message. ~15 lines isn't worth a synthetic resolution key.

On the follow-up for real requiredness

The body says this needs "establishing a source of truth (e.g. deriving requiredness from Python execute() signatures)". Partly true, partly not:

• test/test_actions_sig.py:94 already does inspect.signature(func) against actions.json, and generate_video.execute declares all 12 params with no defaults — so "no default ⇒ required" is derivable for a test.
• But not at request time: submission_validation.py imports only json / os / re + model_allowlist, and deriving requiredness in the front door would mean importing every action module (and google-genai with them) into the request path.

So the body's other suggestion — "required": true in actions.json — is the right cheap source of truth. Just don't carry the "we have no source of truth" framing into the follow-up.

Two more notes

• Code shift: generate_audio: 'true' on an audio_always_on model returned AUDIO_REQUIRED before and now returns MALFORMED_SUBMISSION (the bool check precedes the audio_always_on block). The module docstring calls these codes "stable". Verified nothing branches on them — no test, and grep AUDIO_REQUIRED ui/src is empty — so impact is nil; flagging the contract only.
• Diff-rendering trap, no bug: in gh pr diff the union for duration loop looks nested inside for durations in duration_by_resolution.values(), which would make all_allowed incomplete on the first pass and false-reject. Reading the file on the branch, it is correctly outside the loop.

Spec caveat: I could not locate SM-11 (gh issue list --search "SM-11" --state all returns empty, and the in-repo review reports use BUG-nn / CM-SM-nnn), so the spec axis here is judged against the PR's own title and body.

[christophervoelpel]

Consolidated Review

Verdict: Merge after a retitle, no code change needed. This note reconciles the 19:50 comment on this PR against head a4314fe, and every item below was re-verified by tracing callers (actions_wrapper.py, generate_video.py, ui/src) and running the tests and probes under Evidence.

Do before merge

1. Retitle the PR. Why: the title says "fail closed when required action parameters are absent," but the diff only widens the duration-cap check for omitted resolution and adds a boolean-type check for generate_audio in util/submission_validation.py; the body defers required-parameter enforcement (SM-11) to a follow-up. Acceptance: title names the actual scope, and the description or a linked issue keeps SM-11 explicitly open.

Nothing else is required before merge.

Optional, does not block

• Move the generate_audio boolean check out of the per-model loop in _capability_violation (called at line 626 inside the model_list loop at line 615) so it stops contradicting the docstring at lines 162-165. Idempotent, so a contract/hygiene fix, not a correctness bug.
• Add a regression test for the union hole: duration_seconds=4 with resolution omitted is accepted for the veo model, though models.json only allows duration 8 at 4k. Reproduced live; currently unreachable because any resolution-omitted submission already fails at the worker with a caught TypeError.
• Interpolate the offending value into the generate_audio error message and name the allowed durations in the union-branch message.
• Trim the three tests that pass unchanged against base code (test_duration_over_cap_rejected_when_resolution_present, test_full_valid_submission_passes, test_bool_generate_audio_passes); keep the three that fail on base.
• Fold _FULL_VALID_VIDEO from _VALID_VIDEO via dict spread; only 2 of 8 keys overlap today, so this is minor.

Rejected or superseded, do not re-litigate

• Driving the boolean check off actions.json's declared type: _action_params() returns a set of names only, no type dict, so this needs a new accessor for one field. Sound as written; no action needed.
• Merging the pairwise and union duration branches: they produce genuinely different, resolution-scoped versus union error messages; collapsing them loses information for roughly 15 lines saved.
• The AUDIO_REQUIRED to MALFORMED_SUBMISSION code shift for generate_audio: 'true' on audio-always-on models: both codes are unused anywhere in ui/src, so the shift is real but inert.
• The "nested loop" reading of the union duration branch in gh pr diff: read directly, the loops are siblings inside the same else: block, not nested; a diff-rendering artifact only.

Evidence

• TZ=UTC python -m pytest -q test/test_submission_validation.py at head: 161 passed.
• Base-vs-head reclassification: appended the new test block to a clean git archive of origin/main, ran the 6 disputed tests: 6 passed, 8 failed, matching the claimed keeper/redundant split.
• Live probe importing the test helpers directly: duration_seconds=4, resolution omitted, veo model, validate_submission returned None (accepted), confirming the union hole.
• Traced actions_wrapper.py:_generic_function_caller (line 122) and generate_video.execute (12 required params, no defaults): a missing resolution raises TypeError, caught by a broad exception handler and logged, not an uncaught crash.
• grep -rn "AUDIO_REQUIRED\|MALFORMED_SUBMISSION" ui/src: no matches.
• Not run: live GCP or worker execution, browser or UI verification, paid model calls.

Way forward

• Suggested order for the batch: Use an explicit MIME allow-list and support AVIF #196, Clear video source on resolve failure and keep cached blobs #205, Validate video duration limits and audio parameter types #204, Exclude archived candidates from the rendered video #199 and Clamp clip duration to the footage remaining after skip_time #200 first (independent, ready or one small fix each), then Preserve deployed env values on redeploy and accept quoted config #197 and Remove the duplicate GCS signing cache from orch.py #198 once their fixes land, then the stacked children Validate model-returned product and image ids #201 (on Use an explicit MIME allow-list and support AVIF #196), Treat an absent storyboard key as no change #203 (on Remove the duplicate GCS signing cache from orch.py #198) and Guard setup inputs on the real project load error and hydration state #202 (on Exclude archived candidates from the rendered video #199), each rebased onto main after its parent squash-merges and retargeted to main so CI runs.
• Confirm manually that the retitle and description update land before merge, since that is metadata a reviewer approval will not otherwise catch.
• Open or link SM-11 as a follow-up for real required-parameter enforcement, favoring an actions.json "required" field over signature introspection.

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [Jelena0921,
  andersonfreitas]

[christophervoelpel]

Follow-up on Consolidated Review

I addressed the scope mismatch from the Consolidated Review on head a4314fe4c60b.

• Retitled the PR and updated its description to match the actual duration and audio-type validation scope.
• Kept required-parameter enforcement explicitly open as SM-11; it needs a separate contract decision and test plan.

There were no code changes in this follow-up. Verification: TZ=UTC python -m pytest -q test/test_submission_validation.py passed with 161 tests on the unchanged code head.

Current-head GitHub CI is complete: Python 3.11/3.12/3.13, UI build/lint/tests, deploy checks and security scans passed. Conditional zizmor jobs were skipped.

[victor-paunescu]

Minor improvement for a test.

[victor-paunescu]

Since commit 0d6a560 hoisted the generate_audio boolean check before the model loop specifically so that non-boolean values (such as 1 or 'true') return MALFORMED_SUBMISSION rather than falling through to _capability_violation's AUDIO_REQUIRED check (if value is not True:) on audio_always_on: true models, adding an assertion with _OMNI here directly locks in that error-code precedence.

Suggested change

	@pytest.mark.parametrize('bad_audio', ('true', 'false', 1, 0, None, [1]))
	def test_non_bool_generate_audio_rejected(bad_audio):
	params = {**_FULL_VALID_VIDEO, 'generate_audio': bad_audio}
	assert _code(_sub('generate_video', params)) == 'MALFORMED_SUBMISSION'
	@pytest.mark.parametrize('bad_audio', ('true', 'false', 1, 0, None, [1]))
	def test_non_bool_generate_audio_rejected(bad_audio):
	params = {**_FULL_VALID_VIDEO, 'generate_audio': bad_audio}
	assert _code(_sub('generate_video', params)) == 'MALFORMED_SUBMISSION'
	assert _code(_sub('generate_video', {**_OMNI, 'generate_audio': bad_audio})) == 'MALFORMED_SUBMISSION'

[christophervoelpel]

Agreed, thanks. I merged #204 on the head you approved (rather than pushing and invalidating the approval) and applied this suggestion in #207, wrapped to 80 columns.

Your assertion catches something the existing test misses. If the type check is moved back after _capability_violation, the existing test still passes 6/6, while the new _OMNI assertion fails 6/6 with AUDIO_REQUIRED.