diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py b/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py index a8af4e5145..8fef470d80 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py +++ b/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), } ) diff --git a/libs/hackbot-runtime/tests/test_phabricator_handler.py b/libs/hackbot-runtime/tests/test_phabricator_handler.py index a667ffec9c..4268ae4607 100644 --- a/libs/hackbot-runtime/tests/test_phabricator_handler.py +++ b/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", } diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index 4a75c79c2a..5205fd1061 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/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): diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index 4c668346f2..ed278845f4 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/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"]}