Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions test/test_submission_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -1115,3 +1115,56 @@ def test_input_files_allows_safe_relative_and_gs_paths():
node_id='root',
)
assert validate_submission(data) is None


# --- SM-11: fail closed on missing required params, duration caps, audio bool --

_FULL_VALID_VIDEO = {
'video_variant_quantity': 1,
'aspect_ratio': '16:9',
'duration_seconds': 8,
'gcp_project': '',
'gcp_location': 'global',
'model': 'veo-3.1-generate-001',
'generate_audio': False,
'resolution': '720p',
}


def test_duration_over_cap_rejected_when_resolution_omitted():
params = {k: v for k, v in _FULL_VALID_VIDEO.items() if k != 'resolution'}
params['duration_seconds'] = 9999
assert _code(_sub('generate_video', params)) == 'DURATION_NOT_ALLOWED'


def test_duration_over_cap_rejected_when_resolution_present():
params = {**_FULL_VALID_VIDEO, 'duration_seconds': 9999}
assert _code(_sub('generate_video', params)) == 'DURATION_NOT_ALLOWED'


def test_full_valid_submission_passes():
assert validate_submission(_sub('generate_video', _FULL_VALID_VIDEO)) is None


def test_duration_non_int_rejected_when_resolution_omitted():
params = {k: v for k, v in _FULL_VALID_VIDEO.items() if k != 'resolution'}
params['duration_seconds'] = '8'
assert _code(_sub('generate_video', params)) == 'DURATION_NOT_ALLOWED'


@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'
Comment on lines +1155 to +1158

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



@pytest.mark.parametrize('good_audio', (True, False))
def test_bool_generate_audio_passes(good_audio):
params = {**_FULL_VALID_VIDEO, 'generate_audio': good_audio}
assert validate_submission(_sub('generate_video', params)) is None


def test_duration_within_union_passes_when_resolution_omitted():
params = {k: v for k, v in _FULL_VALID_VIDEO.items() if k != 'resolution'}
params['duration_seconds'] = 4
assert validate_submission(_sub('generate_video', params)) is None
61 changes: 44 additions & 17 deletions util/submission_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -195,32 +195,52 @@ def _capability_violation(
'RESOLUTION_NOT_ALLOWED',
)

if (
'duration_seconds' in params
and 'duration_by_resolution' in caps
and 'resolution' in params
):
if 'duration_seconds' in params and 'duration_by_resolution' in caps:
duration_by_resolution = caps['duration_by_resolution']
# Pairwise, not a union of what each resolution alone allows: list
# parameters expand as a full cross product, so every (resolution,
# duration) combination the engine would actually run must be checked.
for resolution in _as_values(params['resolution']):
if not isinstance(resolution, str):
continue # can never be a key of the mapping
allowed_durations = duration_by_resolution.get(resolution)
if allowed_durations is None:
continue # not a key of the mapping; the resolution check above owns it
if 'resolution' in params:
# Pairwise, not a union of what each resolution alone allows: list
# parameters expand as a full cross product, so every (resolution,
# duration) combination the engine would actually run must be checked.
for resolution in _as_values(params['resolution']):
if not isinstance(resolution, str):
continue # can never be a key of the mapping
allowed_durations = duration_by_resolution.get(resolution)
if allowed_durations is None:
continue # not a key of the mapping; the resolution check above owns it
for duration in _as_values(params['duration_seconds']):
if (
isinstance(duration, bool)
or not isinstance(duration, int)
or duration not in allowed_durations
):
return (
(
f'Node {node_id!r}: {action} duration_seconds {duration!r} '
f'is not allowed for {model!r} at resolution '
f'{resolution!r}'
),
'DURATION_NOT_ALLOWED',
)
else:
# Resolution omitted: validate duration against the union of all allowed
# durations across all resolutions as a minimal fail-closed floor (rejecting
# out-of-bounds durations such as 9999). This is a deliberate compromise:
# rejecting outright would require treating resolution as mandatory, which
# ui/definitions/actions.json does not express. This union check provides a
# floor rather than the full cross-product guarantee the if-branch provides.
all_allowed = set()
for durations in duration_by_resolution.values():
all_allowed.update(durations)
for duration in _as_values(params['duration_seconds']):
if (
isinstance(duration, bool)
or not isinstance(duration, int)
or duration not in allowed_durations
or duration not in all_allowed
):
return (
(
f'Node {node_id!r}: {action} duration_seconds {duration!r} '
f'is not allowed for {model!r} at resolution '
f'{resolution!r}'
f'is not allowed for {model!r}'
),
'DURATION_NOT_ALLOWED',
)
Expand Down Expand Up @@ -566,6 +586,13 @@ def validate_submission(
continue # only model-parameterized actions get the model/location checks

action_params = _action_params(action, actions_json)
if 'generate_audio' in params:
for value in _as_values(params['generate_audio']):
if not isinstance(value, bool):
return (
f'Node {node_id!r}: {action} generate_audio must be a boolean',
'MALFORMED_SUBMISSION',
)
model_param = _model_param_name(action, actions_json)
model_value = params.get(model_param) if model_param else None
model_list = model_value if isinstance(model_value, list) else [model_value]
Expand Down
Loading