Substitute action references in Bugzilla comments

libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py

@@ -275,11 +275,9 @@ async def apply(self, params: dict[str, Any], ctx: ApplyContext) -> ActionResult
             log.exception("Failed to submit Phabricator diff for bug %s", bug_id)
             return ActionResult.failed(str(exc))
 
-        revision_url = _revision_url(new_revision_id) if new_revision_id else None
         return ActionResult.ok(
             {
                 "revision_id": new_revision_id,
-                "revision_url": revision_url,
-                "url": revision_url,
+                "url": _revision_url(new_revision_id),
             }
         )

libs/hackbot-runtime/tests/test_phabricator_handler.py

@@ -75,7 +75,6 @@ async def test_submit_patch_create_wip_by_default(monkeypatch):
     assert result.status == "applied"
     assert result.result == {
         "revision_id": 555,
-        "revision_url": "https://phabricator.services.mozilla.com/D555",
         "url": "https://phabricator.services.mozilla.com/D555",
     }
 

services/hackbot-api/app/actions_applier.py

@@ -49,15 +49,39 @@ def resolve_placeholders(value: Any, results_by_ref: dict[str, dict]) -> Any:
     action's params, not just at the top level. A placeholder referencing a
     ref that hasn't been applied yet (or lacks that field) is left as-is
     rather than raising — the action then fails downstream with an error a
-    human can actually read, instead of a silent substitution glitch.
+    human can actually read, instead of a silent substitution glitch. Any such
+    unresolved placeholder is logged as an error so the literal text landing in
+    a posted comment doesn't go unnoticed.
     """
     if isinstance(value, str):
 
         def _sub(match: re.Match) -> str:
-            result = results_by_ref.get(match.group(1))
-            if result is None or match.group(2) not in result:
-                return match.group(0)
-            return str(result[match.group(2)])
+            placeholder = match.group(0)
+            ref = match.group(1)
+            field = match.group(2)
+
+            if ref not in results_by_ref:
+                log.warning(
+                    "Unresolved action reference %s: no applied action with "
+                    "ref '%s' (left as-is)",
+                    placeholder,
+                    ref,
+                )
+                return placeholder
+
+            ref_result = results_by_ref[ref]
+            if field not in ref_result:
+                log.warning(
+                    "Unresolved action reference %s: action '%s' has no field "
+                    "'%s' (available: %s) (left as-is)",
+                    placeholder,
+                    ref,
+                    field,
+                    ", ".join(sorted(ref_result)) or "none",
+                )
+                return placeholder
+
+            return str(ref_result[field])
 
         return _PLACEHOLDER_RE.sub(_sub, value)
     if isinstance(value, dict):

services/hackbot-api/tests/test_actions_applier.py

@@ -5,6 +5,7 @@
 `apply_all_pending` path — see app/actions_applier.py.
 """
 
+import logging
 import uuid
 from dataclasses import dataclass, field
 from types import SimpleNamespace
@@ -20,32 +21,34 @@
 
 def test_resolves_known_ref_and_field():
     out = resolve_placeholders(
-        "Fix submitted: {{actions.patch.revision_url}}",
-        {"patch": {"revision_url": "https://phabricator.services.mozilla.com/D1"}},
+        "Fix submitted: {{actions.patch.url}}",
+        {"patch": {"url": "https://phabricator.services.mozilla.com/D1"}},
     )
     assert out == "Fix submitted: https://phabricator.services.mozilla.com/D1"
 
 
-def test_unknown_ref_left_as_is():
-    out = resolve_placeholders("See {{actions.missing.url}}", {})
+def test_unknown_ref_left_as_is(caplog):
+    with caplog.at_level(logging.WARNING):
+        out = resolve_placeholders("See {{actions.missing.url}}", {})
     assert out == "See {{actions.missing.url}}"
+    assert "Unresolved action reference {{actions.missing.url}}" in caplog.text
 
 
-def test_unknown_field_left_as_is():
-    out = resolve_placeholders(
-        "See {{actions.patch.nope}}", {"patch": {"revision_url": "x"}}
-    )
+def test_unknown_field_left_as_is(caplog):
+    with caplog.at_level(logging.WARNING):
+        out = resolve_placeholders(
+            "See {{actions.patch.nope}}", {"patch": {"url": "x"}}
+        )
     assert out == "See {{actions.patch.nope}}"
+    assert "Unresolved action reference {{actions.patch.nope}}" in caplog.text
 
 
 def test_recurses_into_dict_and_list():
     value = {
-        "text": "{{actions.patch.revision_url}}",
+        "text": "{{actions.patch.url}}",
         "items": ["{{actions.patch.revision_id}}", "plain"],
     }
-    out = resolve_placeholders(
-        value, {"patch": {"revision_url": "u", "revision_id": 5}}
-    )
+    out = resolve_placeholders(value, {"patch": {"url": "u", "revision_id": 5}})
     assert out == {"text": "u", "items": ["5", "plain"]}