From a6009ed9304fbd2bbadecb8a233a168b3ee5a703 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Tue, 21 Jul 2026 14:19:29 -0400 Subject: [PATCH 1/2] Remove the now-redundant revision_url from the submit_patch result Upstream #6334 added a `url` key to the SubmitPatchHandler result (the key the submit_patch tool documents for `{{actions..url}}`), leaving `revision_url` as a duplicate that nothing consumes. Drop it so the result exposes a single canonical `url`. --- .../actions/handlers/phabricator_handler.py | 4 +--- .../tests/test_phabricator_handler.py | 1 - services/hackbot-api/tests/test_actions_applier.py | 14 +++++--------- 3 files changed, 6 insertions(+), 13 deletions(-) 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/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index 4c668346f2..5d6fd0bf4f 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -20,8 +20,8 @@ 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" @@ -32,20 +32,16 @@ def test_unknown_ref_left_as_is(): def test_unknown_field_left_as_is(): - out = resolve_placeholders( - "See {{actions.patch.nope}}", {"patch": {"revision_url": "x"}} - ) + out = resolve_placeholders("See {{actions.patch.nope}}", {"patch": {"url": "x"}}) assert out == "See {{actions.patch.nope}}" 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"]} From 5cd422f17ed1482e60a98bad3463ea91383f4033 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Thu, 23 Jul 2026 16:10:18 -0400 Subject: [PATCH 2/2] Log a warning when an action reference is left unresolved resolve_placeholders leaves an unresolved {{actions..}} as-is by design, but until now did so silently, so a literal placeholder posted to a comment went unnoticed. Log a warning naming the missing ref or field so it surfaces in the run logs. --- services/hackbot-api/app/actions_applier.py | 34 ++++++++++++++++--- .../hackbot-api/tests/test_actions_applier.py | 15 +++++--- 2 files changed, 40 insertions(+), 9 deletions(-) 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 5d6fd0bf4f..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 @@ -26,14 +27,20 @@ def test_resolves_known_ref_and_field(): 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": {"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():