fix(dify-builder): ground only credentials and host endpoints, never data inputs

The test form and mock data skipped every start variable an http node
read, so a search query or a language header was never mocked. Only a
host-position URL template, credential-keyed header/param lines and an
api-key auth now count, with one shared is_credential_key in core. A
bare "key" param, an empty credential and a bare "Bearer" are grounded
too, and an existing Dify template is never re-grounded.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

api/core/dify_builder/credentials.py

@@ -0,0 +1,70 @@
+"""Whether a header/param/field key names a credential.
+
+Pure, no I/O, stdlib-only -- ``core/dify_builder`` must not import from
+``services``, so this is the single shared home for ``is_credential_key``
+(moved here in the Task 3 review fix round) and its params-only sibling
+``is_credential_param_key``. Both ``core/dify_builder/handlers_fix.py``
+(deciding which start variables a mocked/prefilled form must skip) and
+``services/dify_builder/agent/build.py`` (deciding which literals to ground)
+need the SAME rule, so there is one definition; ``user_supplied.py`` imports
+and re-exports both so existing ``services``-side callers see no API change.
+"""
+
+import re
+
+# A dict/field key's last word-segment that, by itself, names a credential
+# regardless of what comes before it (so "access_token" and "auth" both
+# qualify, but "token_limit" does not -- "limit" is the last segment).
+_CREDENTIAL_LAST_SEGMENTS = {"authorization", "auth", "token", "secret", "password", "passwd"}
+
+# A last segment of "key" only counts as a credential when the segment (or
+# merged word) before it names what kind of key it is -- otherwise ordinary
+# fields like "key_points" would match on "key" alone.
+_CREDENTIAL_KEY_PREFIXES = {"api", "access", "secret", "private", "auth"}
+
+# Splits an identifier into camelCase words: an uppercase run immediately
+# followed by "Xy" (e.g. the "API" in "APIKey"), or an optional leading
+# capital plus a run of lowercase/digits (e.g. "api", "Key", "3").
+_CAMEL_SEGMENT_RE = re.compile(r"[A-Z]+(?=[A-Z][a-z])|[A-Z]?[a-z0-9]+|[A-Z]+|[0-9]+")
+
+
+def _key_segments(key: str) -> list[str]:
+    """Lowercase word-segments of a dict/field key, split on ``_``, ``-``,
+    ``.``, whitespace, and camelCase boundaries -- so ``x-api-key``,
+    ``apiKey`` and ``api_key`` all segment to ``["api", "key"]``."""
+    segments: list[str] = []
+    for part in re.split(r"[_\-.\s]+", key or ""):
+        if not part:
+            continue
+        segments.extend(match.lower() for match in _CAMEL_SEGMENT_RE.findall(part))
+    return segments
+
+
+def is_credential_key(key: str) -> bool:
+    """True iff ``key`` names a credential, decided by its LAST segment --
+    not by containing a credential-ish substring anywhere. So
+    ``password_policy``, ``token_limit``, ``session_token_expiry`` and
+    ``max_tokens`` are NOT credential keys, but ``api_key``, ``apiKey``,
+    ``x-api-key``, ``access_token`` and ``Authorization`` are.
+    """
+    segments = _key_segments(key)
+    if not segments:
+        return False
+    last = segments[-1]
+    if last in _CREDENTIAL_LAST_SEGMENTS or last == "apikey":
+        return True
+    return last == "key" and len(segments) >= 2 and segments[-2] in _CREDENTIAL_KEY_PREFIXES
+
+
+def is_credential_param_key(key: str) -> bool:
+    """``is_credential_key`` PLUS a bare ``key`` (case-insensitive, the WHOLE
+    key -- not merely its last segment): a raw ``?key=...`` query parameter is
+    a common way an API names its API key, which ``is_credential_key`` alone
+    misses (its last-segment rule requires a qualifying prefix like
+    ``api``/``access`` before a bare ``key``). Params-only -- a header simply
+    named ``key`` is not a realistic credential header the way a query param
+    is, so this rule is not applied to headers. ``cache_key``/``key_points``
+    still segment to more than just ``key`` and stay non-credential, same as
+    under ``is_credential_key``.
+    """
+    return is_credential_key(key) or (key or "").strip().lower() == "key"

api/core/dify_builder/handlers_fix.py

@@ -30,6 +30,7 @@ import re
 import uuid
 from typing import Any
 
+from core.dify_builder import credentials
 from core.dify_builder.changes import describe_changed_nodes
 from core.dify_builder.contract import (
     AssistantTurnItem,
@@ -460,22 +461,64 @@ def upload_variable_names(schema: StartSchema) -> set[str]:
 
 
 # ``{{#<node_id>.<var>#}}`` -- the Dify template reference syntax -- may sit
-# anywhere inside a URL string (e.g. ``https://x.com/{{#s.path#}}``), so this
-# searches the whole string rather than anchoring to it.
+# anywhere inside a string (e.g. ``https://x.com/{{#s.path#}}``), so this
+# searches the whole string rather than anchoring to it, except where a
+# caller below deliberately anchors it with ``.match``.
 _ENDPOINT_TEMPLATE_RE = re.compile(r"\{\{#([^.#{}]+)\.([^.#{}]+)#\}\}")
 
 
+def _start_var_names(text: str, start_id: str) -> set[str]:
+    """Every start-node template's variable name occurring ANYWHERE in
+    ``text`` -- used where the field IS the credential/endpoint itself (an
+    ``authorization.config.api_key``, or the value half of a credential-keyed
+    header/param line), so position within it doesn't matter."""
+    return {var_name for node_id, var_name in _ENDPOINT_TEMPLATE_RE.findall(text or "") if node_id == start_id}
+
+
+def _leading_start_var_name(text: str, start_id: str) -> str | None:
+    """The variable name of a start-node template occupying the HOST
+    position: ``text``, stripped of whitespace, STARTS with the template --
+    not merely containing one later, e.g. in a path or query
+    (``https://wttr.in/{{#s.city#}}``'s ``city`` is an ordinary data input,
+    not an endpoint). Returns None when there is no such leading template.
+    """
+    match = _ENDPOINT_TEMPLATE_RE.match((text or "").strip())
+    if match and match.group(1) == start_id:
+        return match.group(2)
+    return None
+
+
+def _credential_line_var_names(text: str, start_id: str, key_is_credential) -> set[str]:
+    """Start-variable templates in the VALUE half of each ``Key: Value`` line
+    of a ``headers``/``params`` text block whose KEY names a credential per
+    ``key_is_credential`` -- an ordinary data field read via a header/param
+    (a search query, an ``Accept-Language``) must stay mockable, so only a
+    credential-keyed line's value counts."""
+    names: set[str] = set()
+    for line in (text or "").split("\n"):
+        if ":" not in line:
+            continue
+        key, _sep, value = line.partition(":")
+        if key_is_credential(key.strip()):
+            names |= _start_var_names(value, start_id)
+    return names
+
+
 def endpoint_variable_names(graph: Graph) -> set[str]:
-    """Names of the start variables an http-request node reads its URL,
-    headers, params, or API-key authorization from.
+    """Names of the start variables an http-request node reads a real
+    endpoint or credential from: the url's HOST position, a credential-keyed
+    ``headers``/``params`` line, or an ``api-key`` authorization's
+    ``config.api_key``.
 
     Nothing can invent a real endpoint or credential any more than it can
     invent an upload (ESQ1-302 / build.py's ``_ground_placeholder_endpoints``
     and Task 3's credential grounding), so these are values a human still has
-    to supply even when every other field is mocked for them. Only a
-    reference to the graph's own START node counts -- a reference to some
-    other node's output is an ordinary wired value, not an endpoint/credential
-    waiting on the user.
+    to supply even when every other field is mocked for them. An ORDINARY
+    data input the workflow happens to read via the url path/query, or via a
+    non-credential header/param (a search query, an ``Accept-Language``), is
+    NOT collected -- it stays mockable, same as any other start variable.
+    Only a reference to the graph's own START node counts at all -- a
+    reference to some other node's output is an ordinary wired value.
     """
     start_id = ""
     for node in graph.get("nodes", []):
@@ -486,23 +529,21 @@ def endpoint_variable_names(graph: Graph) -> set[str]:
     if not start_id:
         return set()
     names: set[str] = set()
-
-    def _collect(text: str) -> None:
-        for node_id, var_name in _ENDPOINT_TEMPLATE_RE.findall(text):
-            if node_id == start_id:
-                names.add(var_name)
-
     for node in graph.get("nodes", []):
         data = node.get("data") or {}
         if data.get("type") != "http-request":
             continue
-        _collect(str(data.get("url") or ""))
-        _collect(str(data.get("headers") or ""))
-        _collect(str(data.get("params") or ""))
+        leading = _leading_start_var_name(str(data.get("url") or ""), start_id)
+        if leading:
+            names.add(leading)
+        names |= _credential_line_var_names(str(data.get("headers") or ""), start_id, credentials.is_credential_key)
+        names |= _credential_line_var_names(
+            str(data.get("params") or ""), start_id, credentials.is_credential_param_key
+        )
         authorization = data.get("authorization")
         auth_config = authorization.get("config") if isinstance(authorization, dict) else None
-        if isinstance(auth_config, dict):
-            _collect(str(auth_config.get("api_key") or ""))
+        if isinstance(authorization, dict) and authorization.get("type") == "api-key" and isinstance(auth_config, dict):
+            names |= _start_var_names(str(auth_config.get("api_key") or ""), start_id)
     return names
 
 

api/services/dify_builder/agent/build.py

@@ -833,22 +833,44 @@ def _ground_placeholder_endpoints(intents: list[MutationIntent], *, trusted_text
     return grounded
 
 
+# A value that IS one of these words and nothing else (no content after it)
+# is a bare auth-scheme with no credential attached -- ``strip_auth_scheme``
+# alone does not catch this: its regex only strips a scheme word FOLLOWED BY
+# whitespace, so a bare "Bearer" (nothing after it) comes back unstripped,
+# not empty.
+_BARE_SCHEME_WORDS = frozenset({"bearer", "basic", "token"})
+
+# Mirrors user_supplied._TEMPLATE_MARKER: a Dify template reference names a
+# variable, not a literal, so it is never "invented" and must never be
+# re-grounded -- checked BEFORE the placeholder-shape check below, since a
+# grounded variable's own NAME (e.g. "your_api_key") can coincidentally match
+# CREDENTIAL_PLACEHOLDER_RE's "YOUR...KEY" pattern.
+_TEMPLATE_MARKER = "{{#"
+
+
 def _is_invented_credential(value: str, trusted_text: str) -> bool:
     """Mirrors ``_ground_placeholder_endpoints``'s OR-of-two-checks shape, but
     for a credential value (a header/param 'Value' half, or an
-    ``authorization.config.api_key``): an unmistakable placeholder
-    (``CREDENTIAL_PLACEHOLDER_RE``, after stripping a leading ``Bearer
-    ``/``Basic ``/``Token `` scheme) grounds even in placeholder-only mode
-    (``trusted_text == ""``, mirroring how a shaped-like-invented URL grounds
-    with no goal text at all); otherwise, with ``trusted_text`` non-empty, a
-    value that never appears verbatim in it also grounds (S5b-style
-    plausible-looking secret the model invented). An empty value has nothing
-    to ground.
+    ``authorization.config.api_key``):
+
+    1. Already a Dify template reference (contains ``{{#``) -- a real
+       reference, never re-grounded, checked first (see ``_TEMPLATE_MARKER``).
+    2. Empty, or nothing beyond a bare ``Bearer``/``Basic``/``Token`` scheme
+       word -- never a valid credential either way, so it grounds
+       unconditionally.
+    3. An unmistakable placeholder (``CREDENTIAL_PLACEHOLDER_RE``) -- grounds
+       even in placeholder-only mode (``trusted_text == ""``, mirroring how a
+       shaped-like-invented URL grounds with no goal text at all).
+    4. Otherwise, with ``trusted_text`` non-empty, a value that never appears
+       verbatim in it also grounds (S5b-style plausible-looking secret the
+       model invented).
     """
     stripped_value = value.strip()
     remainder = user_supplied.strip_auth_scheme(stripped_value)
-    if not remainder:
+    if _TEMPLATE_MARKER in remainder:
         return False
+    if not remainder or remainder.lower() in _BARE_SCHEME_WORDS:
+        return True
     if user_supplied.CREDENTIAL_PLACEHOLDER_RE.search(remainder):
         return True
     return bool(trusted_text) and not user_supplied.is_user_supplied_secret(stripped_value, trusted_text)
@@ -856,26 +878,34 @@ def _is_invented_credential(value: str, trusted_text: str) -> bool:
 
 def _replace_credential_value(value: str, template_ref: str) -> str:
     """Rebuild a header/param 'Value' half with its secret swapped for
-    ``template_ref``, preserving surrounding whitespace and a leading
-    ``Bearer ``/``Basic ``/``Token `` auth-scheme prefix exactly as written
-    (e.g. ``" Bearer YOUR_API_KEY"`` -> ``" Bearer {{#s.h_api_key#}}"``)."""
-    lstripped = value.lstrip(" \t")
+    ``template_ref``, preserving surrounding whitespace (including a ``\\r``
+    left by a ``\\r\\n`` line ending) and a leading ``Bearer ``/``Basic ``/
+    ``Token `` auth-scheme prefix exactly as written (e.g. ``" Bearer
+    YOUR_API_KEY"`` -> ``" Bearer {{#s.h_api_key#}}"``). A BARE scheme word
+    with nothing after it (``"Bearer"``) keeps the word, adding the
+    separating space the grounded value now needs."""
+    lstripped = value.lstrip(" \t\r")
     leading_ws = value[: len(value) - len(lstripped)]
-    core = lstripped.rstrip(" \t")
+    core = lstripped.rstrip(" \t\r")
     trailing_ws = lstripped[len(core) :]
     remainder = user_supplied.strip_auth_scheme(core)
-    scheme_prefix = core[: len(core) - len(remainder)]
+    if remainder == core and core.lower() in _BARE_SCHEME_WORDS:
+        scheme_prefix = f"{core} "
+    else:
+        scheme_prefix = core[: len(core) - len(remainder)]
     return f"{leading_ws}{scheme_prefix}{template_ref}{trailing_ws}"
 
 
-def _ground_credential_text(text: str, template_ref: str, *, trusted_text: str) -> tuple[str, bool]:
+def _ground_credential_text(text: str, template_ref: str, *, trusted_text: str, key_is_credential) -> tuple[str, bool]:
     """Ground each ``Key: Value`` line of an http-request ``headers``/
-    ``params`` text block whose key names a credential
-    (``user_supplied.is_credential_key``, decided by the key's LAST segment)
-    and whose value the user didn't supply (``_is_invented_credential``),
-    replacing only the secret part of that line with ``template_ref``. Every
-    other line -- including a credential line the user DID supply -- passes
-    through byte-identical. Returns ``(new_text, changed)``.
+    ``params`` text block whose key names a credential per
+    ``key_is_credential`` (``user_supplied.is_credential_key`` for headers;
+    ``user_supplied.is_credential_param_key`` for params, which also treats a
+    bare ``key`` as one) and whose value the user didn't supply
+    (``_is_invented_credential``), replacing only the secret part of that
+    line with ``template_ref``. Every other line -- including a credential
+    line the user DID supply -- passes through byte-identical. Returns
+    ``(new_text, changed)``.
     """
     if not text:
         return text, False
@@ -887,7 +917,7 @@ def _ground_credential_text(text: str, template_ref: str, *, trusted_text: str)
             new_lines.append(line)
             continue
         key, _sep, value = line.partition(":")
-        if not user_supplied.is_credential_key(key.strip()) or not _is_invented_credential(value, trusted_text):
+        if not key_is_credential(key.strip()) or not _is_invented_credential(value, trusted_text):
             new_lines.append(line)
             continue
         new_lines.append(f"{key}:{_replace_credential_value(value, template_ref)}")
@@ -936,14 +966,20 @@ def _ground_placeholder_credentials(intents: list[MutationIntent], *, trusted_te
         changed = False
 
         new_headers, headers_changed = _ground_credential_text(
-            str(node_config.get("headers") or ""), template_ref, trusted_text=trusted_text
+            str(node_config.get("headers") or ""),
+            template_ref,
+            trusted_text=trusted_text,
+            key_is_credential=user_supplied.is_credential_key,
         )
         if headers_changed:
             node_config["headers"] = new_headers
             changed = True
 
         new_params, params_changed = _ground_credential_text(
-            str(node_config.get("params") or ""), template_ref, trusted_text=trusted_text
+            str(node_config.get("params") or ""),
+            template_ref,
+            trusted_text=trusted_text,
+            key_is_credential=user_supplied.is_credential_param_key,
         )
         if params_changed:
             node_config["params"] = new_params

api/services/dify_builder/agent/user_supplied.py

@@ -19,6 +19,19 @@ from collections.abc import Mapping
 from typing import Any
 from urllib.parse import urlsplit
 
+from core.dify_builder.credentials import is_credential_key, is_credential_param_key
+
+__all__ = [
+    "CREDENTIAL_PLACEHOLDER_RE",
+    "is_credential_key",
+    "is_credential_param_key",
+    "is_user_supplied_secret",
+    "is_user_supplied_url",
+    "strip_auth_scheme",
+    "trusted_text_for",
+    "url_hosts",
+]
+
 # A URL literal as it appears embedded in freeform prose. Restricted to the
 # ASCII characters RFC 3986 allows in a URI, so it stops before CJK or other
 # non-URL text glued onto the URL with no separating space (e.g. a Chinese
@@ -51,21 +64,6 @@ CREDENTIAL_PLACEHOLDER_RE = re.compile(
     re.IGNORECASE,
 )
 
-# A dict/field key's last word-segment that, by itself, names a credential
-# regardless of what comes before it (so "access_token" and "auth" both
-# qualify, but "token_limit" does not -- "limit" is the last segment).
-_CREDENTIAL_LAST_SEGMENTS = {"authorization", "auth", "token", "secret", "password", "passwd"}
-
-# A last segment of "key" only counts as a credential when the segment (or
-# merged word) before it names what kind of key it is -- otherwise ordinary
-# fields like "key_points" would match on "key" alone.
-_CREDENTIAL_KEY_PREFIXES = {"api", "access", "secret", "private", "auth"}
-
-# Splits an identifier into camelCase words: an uppercase run immediately
-# followed by "Xy" (e.g. the "API" in "APIKey"), or an optional leading
-# capital plus a run of lowercase/digits (e.g. "api", "Key", "3").
-_CAMEL_SEGMENT_RE = re.compile(r"[A-Z]+(?=[A-Z][a-z])|[A-Z]?[a-z0-9]+|[A-Z]+|[0-9]+")
-
 
 def url_hosts(text: str) -> set[str]:
     """Lowercase hostnames of every ``http(s)://…`` URL literally present in
@@ -125,34 +123,6 @@ def is_user_supplied_secret(value: str, trusted_text: str) -> bool:
     return remainder in (trusted_text or "")
 
 
-def _key_segments(key: str) -> list[str]:
-    """Lowercase word-segments of a dict/field key, split on ``_``, ``-``,
-    ``.``, whitespace, and camelCase boundaries -- so ``x-api-key``,
-    ``apiKey`` and ``api_key`` all segment to ``["api", "key"]``."""
-    segments: list[str] = []
-    for part in re.split(r"[_\-.\s]+", key or ""):
-        if not part:
-            continue
-        segments.extend(match.lower() for match in _CAMEL_SEGMENT_RE.findall(part))
-    return segments
-
-
-def is_credential_key(key: str) -> bool:
-    """True iff ``key`` names a credential, decided by its LAST segment --
-    not by containing a credential-ish substring anywhere. So
-    ``password_policy``, ``token_limit``, ``session_token_expiry`` and
-    ``max_tokens`` are NOT credential keys, but ``api_key``, ``apiKey``,
-    ``x-api-key``, ``access_token`` and ``Authorization`` are.
-    """
-    segments = _key_segments(key)
-    if not segments:
-        return False
-    last = segments[-1]
-    if last in _CREDENTIAL_LAST_SEGMENTS or last == "apikey":
-        return True
-    return last == "key" and len(segments) >= 2 and segments[-2] in _CREDENTIAL_KEY_PREFIXES
-
-
 def trusted_text_for(goal_text: str, requirements: Mapping[str, Any]) -> str:
     """The text a candidate URL/credential is checked against: what the user
     actually submitted -- the goal prose plus the requirements form they

api/tests/unit_tests/core/dify_builder/test_handlers_fix.py

@@ -786,17 +786,61 @@ def test_endpoint_variable_names_finds_a_start_variable_referenced_as_the_whole_
     assert endpoint_variable_names(graph) == {"h_url"}
 
 
-def test_endpoint_variable_names_finds_a_start_variable_embedded_in_a_larger_url():
+def test_endpoint_variable_names_ignores_a_url_template_not_in_host_position():
+    """Review fix round 1 (item 1): a start-variable template appearing LATER
+    in the url (path/query), not at its very start, is an ordinary data
+    input -- e.g. ``https://wttr.in/{{#s.city#}}`` -- not an endpoint the
+    user must supply, so mocking ``city`` must not be blocked. Only a
+    template occupying the HOST position (the url, stripped, STARTS with it)
+    counts; this intentionally narrows the old (Plan 3) behaviour, which
+    collected any embedded reference."""
     from core.dify_builder.handlers_fix import endpoint_variable_names
 
     graph = {
         "nodes": [
-            {"id": "s", "data": {"type": "start", "variables": [{"variable": "path", "type": "text-input"}]}},
-            {"id": "h", "data": {"type": "http-request", "url": "https://x.com/{{#s.path#}}"}},
+            {"id": "s", "data": {"type": "start", "variables": [{"variable": "city", "type": "text-input"}]}},
+            {"id": "h", "data": {"type": "http-request", "url": "https://wttr.in/{{#s.city#}}"}},
         ],
         "edges": [],
     }
-    assert endpoint_variable_names(graph) == {"path"}
+    assert endpoint_variable_names(graph) == set()
+
+
+def test_endpoint_variable_names_ignores_non_credential_header_and_param_keys():
+    """Review fix round 1 (item 1): an ordinary data input read via a header
+    or param -- not a secret -- must stay mockable. Before this fix, ANY
+    start-variable template anywhere in headers/params was collected, so a
+    search query or an Accept-Language header was silently dropped from
+    every prefill/mock, and "use mock data" failed for every API-calling
+    app. Only a CREDENTIAL-keyed line counts now."""
+    from core.dify_builder.handlers_fix import endpoint_variable_names
+
+    graph = {
+        "nodes": [
+            {
+                "id": "s",
+                "data": {
+                    "type": "start",
+                    "variables": [
+                        {"variable": "query", "type": "text-input"},
+                        {"variable": "lang", "type": "text-input"},
+                    ],
+                },
+            },
+            {
+                "id": "h",
+                "data": {
+                    "type": "http-request",
+                    "url": "https://api.acme.com/v1/search",
+                    "headers": "Accept-Language: {{#s.lang#}}",
+                    "params": "q: {{#s.query#}}",
+                    "authorization": {"type": "no-auth", "config": None},
+                },
+            },
+        ],
+        "edges": [],
+    }
+    assert endpoint_variable_names(graph) == set()
 
 
 def test_endpoint_variable_names_ignores_a_reference_to_a_non_start_node():

api/tests/unit_tests/services/dify_builder/test_build_nodes.py

@@ -789,3 +789,192 @@ def test_build_nodes_grounds_an_invented_api_key_param():
     assert http.args["config"]["params"] == "api_key: {{#s.h_api_key#}}"
     var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_api_key")
     assert var["required"] is True
+
+
+# ---- Task 3 review fix round 1 ------------------------------------------
+
+
+def test_build_nodes_grounds_a_bare_key_param():
+    """Review item 2: a raw ``?key=...`` param is a common way an API names
+    its key; ``is_credential_key("key")`` alone is False (its last-segment
+    rule needs a qualifying prefix), so params get a params-only rule that
+    also treats a bare, whole ``key`` as a credential."""
+    intents = _build_with(
+        _gen_graph_with_http_full(
+            "https://api.acme.com/v1/render",
+            params="key: YOUR_API_KEY",
+        )
+    )
+
+    http = next(i for i in intents if i.args.get("node_type") == "http-request")
+    start = next(i for i in intents if i.args.get("node_type") == "start")
+    assert http.args["config"]["params"] == "key: {{#s.h_api_key#}}"
+    var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_api_key")
+    assert var["required"] is True
+
+
+def test_build_nodes_leaves_cache_key_and_key_points_params_alone():
+    """The params-only bare-``key`` rule must not fire on a key that merely
+    ENDS in "key" as part of a longer word/segment."""
+    intents = _build_with(
+        _gen_graph_with_http_full(
+            "https://api.acme.com/v1/render",
+            params="cache_key: YOUR_API_KEY\nkey_points: YOUR_API_KEY",
+        )
+    )
+
+    http = next(i for i in intents if i.args.get("node_type") == "http-request")
+    start = next(i for i in intents if i.args.get("node_type") == "start")
+    assert http.args["config"]["params"] == "cache_key: YOUR_API_KEY\nkey_points: YOUR_API_KEY"
+    assert start.args["config"]["variables"] == []
+
+
+def test_build_nodes_grounds_an_empty_api_key_authorization_config():
+    """Review item 3: an empty ``config.api_key`` is never a valid credential
+    either -- it must ground exactly like a placeholder one, not pass through
+    and fail every run."""
+    intents = _build_with(
+        _gen_graph_with_http_full(
+            "https://api.acme.com/v1/render",
+            authorization={"type": "api-key", "config": {"type": "bearer", "api_key": ""}},
+        )
+    )
+
+    http = next(i for i in intents if i.args.get("node_type") == "http-request")
+    start = next(i for i in intents if i.args.get("node_type") == "start")
+    assert http.args["config"]["authorization"]["config"]["api_key"] == "{{#s.h_api_key#}}"
+    var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_api_key")
+    assert var["required"] is True
+
+
+def test_build_nodes_grounds_an_empty_credential_header_line():
+    """Review item 3, header/param side: ``Authorization:`` with nothing
+    after the colon is an empty credential, not a real one -- it must
+    ground too."""
+    intents = _build_with(
+        _gen_graph_with_http_full(
+            "https://api.acme.com/v1/render",
+            headers="Authorization:",
+        )
+    )
+
+    http = next(i for i in intents if i.args.get("node_type") == "http-request")
+    start = next(i for i in intents if i.args.get("node_type") == "start")
+    assert http.args["config"]["headers"] == "Authorization:{{#s.h_api_key#}}"
+    var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_api_key")
+    assert var["required"] is True
+
+
+def test_build_nodes_leaves_an_already_templated_credential_header_alone():
+    """Review item 4: a header that already reads a start variable (e.g.
+    from an earlier grounding pass, or one the model wrote itself) must
+    never be re-grounded -- even though the variable NAME can itself look
+    placeholder-shaped (``your_api_key`` matches CREDENTIAL_PLACEHOLDER_RE's
+    ``YOUR...KEY`` pattern). The template check must run BEFORE the
+    placeholder-shape check."""
+    intents = _build_with(
+        _gen_graph_with_http_full(
+            "https://api.acme.com/v1/render",
+            headers="Authorization: Bearer {{#s.your_api_key#}}",
+        )
+    )
+
+    http = next(i for i in intents if i.args.get("node_type") == "http-request")
+    start = next(i for i in intents if i.args.get("node_type") == "start")
+    assert http.args["config"]["headers"] == "Authorization: Bearer {{#s.your_api_key#}}"
+    assert start.args["config"]["variables"] == []
+
+
+def test_build_nodes_grounds_a_bare_scheme_word_keeping_it():
+    """Review minor (b): a value that is EXACTLY a scheme word (nothing
+    after it -- ``strip_auth_scheme`` only strips a scheme FOLLOWED by
+    whitespace, so a bare word isn't recognised as empty by that helper
+    alone) is grounded while keeping the scheme word."""
+    intents = _build_with(
+        _gen_graph_with_http_full(
+            "https://api.acme.com/v1/render",
+            headers="Authorization: Bearer",
+        )
+    )
+
+    http = next(i for i in intents if i.args.get("node_type") == "http-request")
+    start = next(i for i in intents if i.args.get("node_type") == "start")
+    assert http.args["config"]["headers"] == "Authorization: Bearer {{#s.h_api_key#}}"
+    var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_api_key")
+    assert var["required"] is True
+
+
+def test_build_nodes_grounds_a_credential_header_preserving_crlf_line_endings():
+    """Review minor (a): ``_replace_credential_value`` used to strip only
+    `` \\t``, so a ``\\r\\n``-terminated line's trailing ``\\r`` was treated
+    as part of the secret and silently dropped instead of preserved as a
+    line-ending character."""
+    intents = _build_with(
+        _gen_graph_with_http_full(
+            "https://api.acme.com/v1/render",
+            headers="Content-Type: application/json\r\nAuthorization: Bearer YOUR_API_KEY\r\n",
+        )
+    )
+
+    http = next(i for i in intents if i.args.get("node_type") == "http-request")
+    assert http.args["config"]["headers"] == (
+        "Content-Type: application/json\r\nAuthorization: Bearer {{#s.h_api_key#}}\r\n"
+    )
+
+
+def test_build_nodes_grounds_an_invented_secret_absent_from_trusted_text():
+    """Review minor (c), test 1: in trusted mode, a non-placeholder-shaped
+    but still invented secret (absent from what the user actually typed)
+    must ground -- not just an obviously placeholder-shaped one."""
+    goal = "Call https://api.acme.com/v1/render to render a slide deck."
+    intents = _build_with(
+        _gen_graph_with_http_full(
+            "https://api.acme.com/v1/render",
+            headers="Authorization: Bearer sk-invented",
+        ),
+        trusted_text=goal,
+    )
+
+    http = next(i for i in intents if i.args.get("node_type") == "http-request")
+    start = next(i for i in intents if i.args.get("node_type") == "start")
+    assert http.args["config"]["headers"] == "Authorization: Bearer {{#s.h_api_key#}}"
+    var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_api_key")
+    assert var["required"] is True
+
+
+def test_build_nodes_leaves_a_non_placeholder_secret_alone_in_placeholder_only_mode():
+    """Review minor (c), test 2: with no trusted_text at all, only an
+    unmistakably placeholder-shaped credential grounds -- a real-looking
+    secret is left alone rather than treated as invented just because there
+    is no goal text to check it against (mirrors the URL grounding's own
+    placeholder-only-mode behaviour)."""
+    intents = _build_with(
+        _gen_graph_with_http_full(
+            "https://api.acme.com/v1/render",
+            headers="Authorization: Bearer sk-live-abc123",
+        )
+    )
+
+    http = next(i for i in intents if i.args.get("node_type") == "http-request")
+    start = next(i for i in intents if i.args.get("node_type") == "start")
+    assert http.args["config"]["headers"] == "Authorization: Bearer sk-live-abc123"
+    assert start.args["config"]["variables"] == []
+
+
+def test_build_nodes_grounds_both_an_invented_url_and_an_invented_header_on_the_same_node():
+    """Review minor (c), test 3: a node with both an invented URL and an
+    invented credential header gets BOTH a ``<node>_url`` and a
+    ``<node>_api_key`` start variable -- the two grounding passes share the
+    start-variable insertion write without clobbering each other."""
+    intents = _build_with(
+        _gen_graph_with_http_full(
+            "https://api.example.com/ppt/generate",
+            headers="Authorization: Bearer YOUR_API_KEY",
+        )
+    )
+
+    http = next(i for i in intents if i.args.get("node_type") == "http-request")
+    start = next(i for i in intents if i.args.get("node_type") == "start")
+    assert http.args["config"]["url"] == "{{#s.h_url#}}"
+    assert http.args["config"]["headers"] == "Authorization: Bearer {{#s.h_api_key#}}"
+    assert {v["variable"] for v in start.args["config"]["variables"]} == {"h_url", "h_api_key"}

api/tests/unit_tests/services/dify_builder/test_user_supplied.py

@@ -114,6 +114,35 @@ def test_is_credential_key_false_for_empty_key():
     assert us.is_credential_key("") is False
 
 
+# --- Task 3 review fix round 1: is_credential_key moved into core, plus a
+# params-only sibling that also treats a bare "key" as a credential --------
+
+
+def test_is_credential_key_still_importable_after_moving_into_core():
+    """The pure logic now lives in core.dify_builder.credentials (core must
+    not import services); user_supplied re-exports it so this module's own
+    API is unchanged."""
+    from core.dify_builder.credentials import is_credential_key as core_is_credential_key
+
+    assert us.is_credential_key is core_is_credential_key
+
+
+def test_is_credential_param_key_also_treats_a_bare_key_as_a_credential():
+    assert us.is_credential_param_key("key")
+    assert us.is_credential_param_key("KEY")
+    assert us.is_credential_param_key("  key  ")
+
+
+def test_is_credential_param_key_still_excludes_lookalikes():
+    for key in ("cache_key", "key_points", "password_policy", "token_limit"):
+        assert not us.is_credential_param_key(key), key
+
+
+def test_is_credential_param_key_still_true_for_ordinary_credential_keys():
+    for key in ("api_key", "apiKey", "x-api-key", "access_token", "Authorization"):
+        assert us.is_credential_param_key(key), key
+
+
 # --- Fix round 1: url_hosts stops at the first non-URL character ----------