From 1007e03b20490780ac7c9792fd1cf8dc02afc6ba Mon Sep 17 00:00:00 2001 From: majdyz Date: Sat, 11 Apr 2026 12:07:03 +0000 Subject: [PATCH] refactor(executor/utils): extract credential error markers + parity tests is_credential_validation_error_message was matched against hand-typed strings that had to be kept in sync with the four raise sites inside _validate_node_input_credentials by convention only. Adding a new credential error string would have silently regressed the copilot's credential-race fallback to a plain text error, with nothing to catch the drift at test time. Extract the four error templates to module-level constants (CRED_ERR_REQUIRED, CRED_ERR_INVALID_PREFIX, CRED_ERR_NOT_AVAILABLE_PREFIX, CRED_ERR_UNKNOWN_PREFIX, CRED_ERR_INVALID_TYPE_MISMATCH) used from both raise sites and the matcher, with a _CREDENTIAL_ERROR_MARKERS table that classifies each as exact-match or prefix-match. Add a parity test that asserts each constant-emitted message is recognised by the public matcher (including typical runtime suffixes from the f-strings), plus a case-insensitive check and a negative test. Now adding a new credential error means adding a constant, and the test_credential_error_markers_cover_all_raise_sites test fails if the matcher drifts. --- .../backend/backend/executor/utils.py | 67 +++++++++++----- .../backend/backend/executor/utils_test.py | 79 ++++++++++++++++++- 2 files changed, 127 insertions(+), 19 deletions(-) diff --git a/autogpt_platform/backend/backend/executor/utils.py b/autogpt_platform/backend/backend/executor/utils.py index 61dc8f3a0f..732f0b8d1e 100644 --- a/autogpt_platform/backend/backend/executor/utils.py +++ b/autogpt_platform/backend/backend/executor/utils.py @@ -249,6 +249,37 @@ def validate_exec( return data, node_block.name +# --------------------------------------------------------------------------- +# Credential validation error message templates. +# +# These constants are the single source of truth for the error messages +# emitted by ``_validate_node_input_credentials``. Both the raise sites +# below and the public matcher ``is_credential_validation_error_message`` +# reference them, so adding a new credential error means adding a +# constant here — the matcher and tests stay in sync automatically. +# +# If you add a new credential error string, also add its constant to +# ``_CREDENTIAL_ERROR_MARKERS`` below so the copilot's credential-race +# fallback continues to recognise it. +# --------------------------------------------------------------------------- +CRED_ERR_REQUIRED = "These credentials are required" +CRED_ERR_INVALID_PREFIX = "Invalid credentials:" +CRED_ERR_INVALID_TYPE_MISMATCH = "Invalid credentials: type/provider mismatch" +CRED_ERR_NOT_AVAILABLE_PREFIX = "Credentials not available:" +CRED_ERR_UNKNOWN_PREFIX = "Unknown credentials #" + +# Markers used by ``is_credential_validation_error_message`` to classify a +# message. Each entry is (match_mode, lowercased_marker) — "exact" means +# the full message must equal the marker, "prefix" means it must start +# with the marker. +_CREDENTIAL_ERROR_MARKERS: tuple[tuple[str, str], ...] = ( + ("exact", CRED_ERR_REQUIRED.lower()), + ("prefix", CRED_ERR_INVALID_PREFIX.lower()), + ("prefix", CRED_ERR_NOT_AVAILABLE_PREFIX.lower()), + ("prefix", CRED_ERR_UNKNOWN_PREFIX.lower()), +) + + def is_credential_validation_error_message(message: str) -> bool: """Return True if *message* came from the credential gate in :func:`_validate_node_input_credentials`. @@ -258,17 +289,19 @@ def is_credential_validation_error_message(message: str) -> bool: credential race) can distinguish credential failures from other graph validation errors without redefining the string list. - Adding a new error string in ``_validate_node_input_credentials`` - **must** also be reflected here — otherwise the copilot will fall - back to a plain text error for that case. + Drift prevention: raise sites and this matcher both reference the + ``CRED_ERR_*`` constants defined above, and + ``test_credential_error_markers_cover_all_raise_sites`` exercises + every branch of ``_validate_node_input_credentials`` to assert the + emitted messages are recognised. """ lower = message.lower() - return ( - lower == "these credentials are required" - or lower.startswith("invalid credentials:") - or lower.startswith("credentials not available:") - or lower.startswith("unknown credentials #") - ) + for mode, marker in _CREDENTIAL_ERROR_MARKERS: + if mode == "exact" and lower == marker: + return True + if mode == "prefix" and lower.startswith(marker): + return True + return False async def _validate_node_input_credentials( @@ -333,9 +366,7 @@ async def _validate_node_input_credentials( if field_is_optional: continue # Don't add error, will be marked for skip after loop else: - credential_errors[node.id][ - field_name - ] = "These credentials are required" + credential_errors[node.id][field_name] = CRED_ERR_REQUIRED continue credentials_meta = credentials_meta_type.model_validate(field_value) @@ -343,7 +374,9 @@ async def _validate_node_input_credentials( except ValidationError as e: # Validation error means credentials were provided but invalid # This should always be an error, even if optional - credential_errors[node.id][field_name] = f"Invalid credentials: {e}" + credential_errors[node.id][ + field_name + ] = f"{CRED_ERR_INVALID_PREFIX} {e}" continue try: @@ -356,13 +389,13 @@ async def _validate_node_input_credentials( # If credentials were explicitly configured but unavailable, it's an error credential_errors[node.id][ field_name - ] = f"Credentials not available: {e}" + ] = f"{CRED_ERR_NOT_AVAILABLE_PREFIX} {e}" continue if not credentials: credential_errors[node.id][ field_name - ] = f"Unknown credentials #{credentials_meta.id}" + ] = f"{CRED_ERR_UNKNOWN_PREFIX}{credentials_meta.id}" continue if ( @@ -375,9 +408,7 @@ async def _validate_node_input_credentials( f"{credentials_meta.type}<>{credentials.type};" f"{credentials_meta.provider}<>{credentials.provider}" ) - credential_errors[node.id][ - field_name - ] = "Invalid credentials: type/provider mismatch" + credential_errors[node.id][field_name] = CRED_ERR_INVALID_TYPE_MISMATCH continue # If node has optional credentials and any are missing, allow running without. diff --git a/autogpt_platform/backend/backend/executor/utils_test.py b/autogpt_platform/backend/backend/executor/utils_test.py index e708673756..4b88cf9825 100644 --- a/autogpt_platform/backend/backend/executor/utils_test.py +++ b/autogpt_platform/backend/backend/executor/utils_test.py @@ -7,7 +7,15 @@ from pytest_mock import MockerFixture from backend.data.dynamic_fields import merge_execution_input, parse_execution_output from backend.data.execution import ExecutionStatus, GraphExecutionWithNodes from backend.data.model import User -from backend.executor.utils import add_graph_execution +from backend.executor.utils import ( + CRED_ERR_INVALID_PREFIX, + CRED_ERR_INVALID_TYPE_MISMATCH, + CRED_ERR_NOT_AVAILABLE_PREFIX, + CRED_ERR_REQUIRED, + CRED_ERR_UNKNOWN_PREFIX, + add_graph_execution, + is_credential_validation_error_message, +) from backend.util.mock import MockObject @@ -1023,3 +1031,72 @@ async def test_stop_graph_execution_cascades_to_child_with_reviews( # Verify both parent and child status updates assert mock_execution_db.update_graph_execution_stats.call_count >= 1 + + +# --------------------------------------------------------------------------- +# Credential validation error marker parity. +# +# ``is_credential_validation_error_message`` is shared by the executor +# dry-run path and the copilot credential-race fallback. Adding a new +# credential error string in ``_validate_node_input_credentials`` without +# updating the matcher would silently regress the copilot UX to a plain +# text error. These tests pin the contract: +# +# 1. Every ``CRED_ERR_*`` constant emitted by the raise sites is +# recognised by the public matcher (including reasonable formatted +# variants with runtime suffixes from ``f"{PREFIX} {e}"``). +# 2. The matcher is case-insensitive and unaffected by trailing detail. +# 3. Non-credential messages fall through. +# --------------------------------------------------------------------------- + + +def test_credential_error_markers_cover_all_raise_sites(): + """Each credential error string emitted by + ``_validate_node_input_credentials`` must be recognised by + ``is_credential_validation_error_message``. This guards against + drift when a new credential error is introduced without updating + the matcher.""" + # Exact-match raise sites + assert is_credential_validation_error_message(CRED_ERR_REQUIRED) + assert is_credential_validation_error_message(CRED_ERR_INVALID_TYPE_MISMATCH) + + # Prefix raise sites with typical runtime suffixes (matching the + # f-strings inside ``_validate_node_input_credentials``) + assert is_credential_validation_error_message( + f"{CRED_ERR_INVALID_PREFIX} 1 validation error for ApiKeyCredentials" + ) + assert is_credential_validation_error_message( + f"{CRED_ERR_NOT_AVAILABLE_PREFIX} connection refused" + ) + assert is_credential_validation_error_message( + f"{CRED_ERR_UNKNOWN_PREFIX}abc-123-def" + ) + + +def test_credential_error_marker_matching_is_case_insensitive(): + """The matcher lowercases inputs before comparing — ensure that + stays true for each marker so log-normalised copies still match.""" + assert is_credential_validation_error_message(CRED_ERR_REQUIRED.upper()) + assert is_credential_validation_error_message(CRED_ERR_REQUIRED.lower()) + assert is_credential_validation_error_message( + f"{CRED_ERR_INVALID_PREFIX.upper()} BAD FIELD" + ) + assert is_credential_validation_error_message( + f"{CRED_ERR_UNKNOWN_PREFIX.upper()}XYZ" + ) + + +def test_non_credential_errors_are_not_matched(): + """Unrelated graph validation errors must not hit the credential + branch — otherwise the copilot would hide structural errors behind + the credential setup card.""" + assert not is_credential_validation_error_message("") + assert not is_credential_validation_error_message( + "missing input {'required_field'}" + ) + assert not is_credential_validation_error_message("Input field 'url' is required") + # A message that happens to contain "credentials" somewhere but + # doesn't start with any known prefix must not match. + assert not is_credential_validation_error_message( + "Block configuration says credentials are fine" + )