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.
This commit is contained in:
majdyz
2026-04-11 12:07:03 +00:00
parent 260bbb28bd
commit 1007e03b20
2 changed files with 127 additions and 19 deletions

View File

@@ -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.

View File

@@ -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"
)