From a4314fe4c60b59d8bde0da1e094860f4205c902f Mon Sep 17 00:00:00 2001 From: christophervoelpel <123032885+christophervoelpel@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:40:21 +0000 Subject: [PATCH 1/2] Fail closed when required action parameters are absent Validate duration capability constraints even when resolution is omitted by checking duration against the union of allowed durations across all supported resolutions for the model. Also validate that generate_audio, when present, is a boolean. When resolution is omitted, the union check provides a fail-closed floor (rejecting out-of-bounds durations such as 9999) rather than skipping the check. Outright rejection of omitted resolution is not applied here because actions.json does not express parameter requiredness; deriving requiredness from execute() signatures or schema annotations should be addressed in a dedicated follow-up. Impact clarification: earlier reviews overstated omission of resolution as a Veo cost vector. Omitting resolution is not a cost risk because generate_video.execute raises TypeError before any Veo call is dispatched. The actual defect is a fail-open contract violation and wasted worker queue work. TAG=agy CONV=1ece3a5d-2611-4c94-b69e-9e85c2753833 --- test/test_submission_validation.py | 47 ++++++++++++++++++++++ util/submission_validation.py | 62 ++++++++++++++++++++++-------- 2 files changed, 92 insertions(+), 17 deletions(-) diff --git a/test/test_submission_validation.py b/test/test_submission_validation.py index fea42c0..adfc292 100644 --- a/test/test_submission_validation.py +++ b/test/test_submission_validation.py @@ -1115,3 +1115,50 @@ 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' + + +@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 diff --git a/util/submission_validation.py b/util/submission_validation.py index 801012a..026e28a 100644 --- a/util/submission_validation.py +++ b/util/submission_validation.py @@ -195,36 +195,64 @@ 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', ) + 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', + ) + if caps.get('audio_always_on') is True and 'generate_audio' in action_params: if 'generate_audio' not in params: return ( From 0d6a560b37fb2d6d8b6080c173240fbcce0a48ea Mon Sep 17 00:00:00 2001 From: christophervoelpel <123032885+christophervoelpel@users.noreply.github.com> Date: Tue, 22 Sep 2026 07:48:12 +0000 Subject: [PATCH 2/2] Hoist generate_audio bool check before model loop and add union-floor test --- test/test_submission_validation.py | 22 ++++++++++++++-------- util/submission_validation.py | 15 +++++++-------- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/test/test_submission_validation.py b/test/test_submission_validation.py index adfc292..3179701 100644 --- a/test/test_submission_validation.py +++ b/test/test_submission_validation.py @@ -1120,14 +1120,14 @@ def test_input_files_allows_safe_relative_and_gs_paths(): # --- 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', + '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', } @@ -1162,3 +1162,9 @@ def test_non_bool_generate_audio_rejected(bad_audio): 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 diff --git a/util/submission_validation.py b/util/submission_validation.py index 026e28a..2bbfd22 100644 --- a/util/submission_validation.py +++ b/util/submission_validation.py @@ -245,14 +245,6 @@ def _capability_violation( 'DURATION_NOT_ALLOWED', ) - 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', - ) - if caps.get('audio_always_on') is True and 'generate_audio' in action_params: if 'generate_audio' not in params: return ( @@ -594,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]