From 58b42a6849d55f65635af11e6307521607c3b305 Mon Sep 17 00:00:00 2001 From: Jared Wein Date: Thu, 6 Aug 2026 15:34:08 -0400 Subject: [PATCH] Auto-apply frontend-triage results only at high confidence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit frontend-triage produces a root-cause analysis and fix plan and records it as Bugzilla actions, but nothing applied them: a human had to click Apply in the hackbot UI. Close that loop for the results the agent is confident about. `AgentSpec` gains `auto_apply_confidence` — the `findings.confidence` levels whose actions may be applied unattended — and frontend-triage opts in at `{"high"}` only, so widening the policy later is an edit to that set rather than to the applier. Because `confidence` is parsed out of the agent's free-form JSON block, it is compared case- and whitespace-insensitively, and a run reporting nothing usable never qualifies. Confidence gates the agent's judgement, not its reach. An action's params are model output and the apply step dispatches them against the runtime's *global* handler registry — which can create bugs, attach files and write to Phabricator — so restricting which tools the agent was given does not restrict what its recorded actions reach. `auto_apply_guard` bounds that: for triage, one comment and one add-only `keywords`/`severity` change on the bug the run was asked about. Anything else holds the whole run, since the comment explains the field change and the two are coalesced into one PUT. The agent is now told what its rating causes, because it was being asked to self-report a control input without knowing it was one. Medium and low results are unchanged: still recorded, still visible in the UI, still appliable by hand. So are all the other agents. --- agents/frontend-triage/README.md | 16 +- .../hackbot_agents/frontend_triage/config.py | 4 + .../frontend_triage/rules/frontend-triage.md | 9 +- .../hackbot_runtime/actions/bugzilla.py | 9 +- services/hackbot-api/app/actions_applier.py | 94 +++- services/hackbot-api/app/agents.py | 14 + services/hackbot-api/app/auto_apply.py | 111 +++++ services/hackbot-api/app/schemas.py | 27 ++ .../hackbot-api/tests/test_actions_applier.py | 434 +++++++++++++++++- 9 files changed, 687 insertions(+), 31 deletions(-) create mode 100644 services/hackbot-api/app/auto_apply.py diff --git a/agents/frontend-triage/README.md b/agents/frontend-triage/README.md index 3dbcba945f..2285913176 100644 --- a/agents/frontend-triage/README.md +++ b/agents/frontend-triage/README.md @@ -99,14 +99,24 @@ Each run writes to `~/hackbot/artifacts//`: - **`summary.json`** — `findings` holds the structured plan (`root_cause`, `proposed_fix`, `target_files`, `confidence`) plus the executor handoff fields - `actionable`, `regressor_node` and `relevant_tests`. `actions` holds the single - **recorded** `bugzilla.add_comment` — written here for review, not posted. + `actionable`, `regressor_node` and `relevant_tests`. `actions` holds the + **recorded** `bugzilla.add_comment` (and, at high confidence, possibly a + `bugzilla.update_bug`). Recording is not posting — but see below: hackbot posts + a high-confidence run's actions to the bug unattended. - **`logs/agent.log`** — the streamed reasoning and every tool call, and the only record of which model actually ran. - **No `changes/` directory.** Its absence confirms the run stayed read-only. -Two things to know before acting on a plan: +Three things to know before acting on a plan: +- **`confidence` decides whether the actions are posted.** A `high`-confidence + run has its recorded actions applied to the real bug automatically; `medium` + and `low` are held for a human to apply from the hackbot UI. Only the fields + the agent is trusted with (`keywords`, `severity`) can go up unattended, and + only against the bug the run was asked about — anything else holds the whole + run for review. See `auto_apply_confidence` in + `services/hackbot-api/app/agents.py` and `frontend_triage_guard` in + `services/hackbot-api/app/auto_apply.py`. - **`confidence` describes the diagnosis, not the fix.** It reflects how clearly the agent pinned a root cause in the code, never whether the fix works — it cannot run anything. Read `high` as "trust the diagnosis, still review the diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py index 9364c0a901..a4a52bbb78 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/config.py +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/config.py @@ -33,6 +33,10 @@ # and plans only: it records a comment with its findings/plan and, at high # confidence, may propose field updates (e.g. keyword/severity). It never # creates bugs or attaches files. +# +# `bugzilla.update_bug` needs `editbugs` on the apply account. The apply step coalesces +# a same-bug field change with the nearest comment into one PUT, so losing that +# privilege would take the analysis comment down with the rejected field change. ENABLED_ACTION_TYPES = [ "bugzilla.add_comment", "bugzilla.update_bug", diff --git a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md index cef0da309c..7c6dc47ff7 100644 --- a/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md +++ b/agents/frontend-triage/hackbot_agents/frontend_triage/rules/frontend-triage.md @@ -36,10 +36,17 @@ not restate the whole bug. Do not claim the fix is verified — you did not run ## Confidence and field changes +Your `confidence` decides whether your actions reach the bug: a **high**-confidence +run is applied to Bugzilla automatically, while **medium** and **low** are held for +a human to review first. Reserve `high` for when you have actually localized the +cause in specific code — not for a plausible-sounding hypothesis. + - **High** (you found the specific code and the cause is clear): record the plan comment. If a rule or convention clearly applies, you may also record a `bugzilla_update_bug` for an obviously-correct field (e.g. adding a relevant - keyword). Do not change `status`/`resolution`. + keyword). Do not change `status`/`resolution`. Record a keyword addition as + `{"keywords": {"add": ["…"]}}` — a bare list replaces every keyword already on + the bug, so it is held for review rather than applied. - **Medium** (plausible area, cause not pinned down): record the comment with your best hypothesis and the open questions that would confirm it. - **Low** (could not localize): record a comment stating what you checked and diff --git a/libs/hackbot-runtime/hackbot_runtime/actions/bugzilla.py b/libs/hackbot-runtime/hackbot_runtime/actions/bugzilla.py index c8087b298b..15f265b401 100644 --- a/libs/hackbot-runtime/hackbot_runtime/actions/bugzilla.py +++ b/libs/hackbot-runtime/hackbot_runtime/actions/bugzilla.py @@ -64,7 +64,9 @@ async def update_bug( ) -> str: """Record an intended change to a Bugzilla bug. - Recorded into the run summary for human review — does not modify Bugzilla. + Recorded into the run summary rather than sent from here. Some agents have their + recorded changes applied without human review, so record only changes you would + stand behind unreviewed. """ recorder.record( "bugzilla.update_bug", @@ -92,8 +94,9 @@ async def add_comment( ) -> str: """Record an intended comment on a bug. - Use is_private=true for security-sensitive notes. Recorded into the run - summary for human review — does not post to Bugzilla. + Use is_private=true for security-sensitive notes. Recorded into the run summary + rather than posted from here. Some agents have their recorded comments posted + without human review, so write it as if it will be read on the bug unreviewed. """ text_with_footer = text.rstrip() + "\n\n" + _COMMENT_FOOTER recorder.record( diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index 49f2f020ce..b79930bff7 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/services/hackbot-api/app/actions_applier.py @@ -2,12 +2,12 @@ On run completion the recorded actions from `summary["actions"]` are always upserted as `run_actions` rows (one per entry) so they're visible and -manageable in the UI. Whether they're then applied *automatically* depends on -the agent's `auto_apply_actions` opt-in (see `app/agents.py`); either way they -can be applied on demand (manual apply-all from the UI). Application runs each -pending row through the handler registry in `hackbot_runtime.actions.handlers` -and is idempotent per action — an already-`applied` row is never re-applied, so -Pub/Sub retries and repeated manual applies are safe. +manageable in the UI. Whether they're then applied *automatically* is decided by +`_should_auto_apply` (see `app/agents.py`); either way they can be applied on demand +(manual apply-all from the UI). Application runs each pending row through the handler +registry in `hackbot_runtime.actions.handlers` and is idempotent per action — an +already-`applied` row is never re-applied, so Pub/Sub retries and repeated manual +applies are safe. """ from __future__ import annotations @@ -28,9 +28,9 @@ from sqlalchemy.ext.asyncio import AsyncSession from app import gcs -from app.agents import AGENT_REGISTRY +from app.agents import AGENT_REGISTRY, AgentSpec from app.database.models import Run, RunAction -from app.schemas import RunStatus +from app.schemas import Confidence, RunStatus, parse_confidence log = logging.getLogger(__name__) @@ -86,6 +86,58 @@ def _sub(match: re.Match) -> str: return value +def _reported_confidence(run: Run) -> Confidence | None: + """The run's self-reported confidence, or None if it didn't report a usable one.""" + return parse_confidence( + ((run.summary or {}).get("findings") or {}).get("confidence") + ) + + +def _should_auto_apply( + spec: AgentSpec | None, run: Run, rows: list[tuple[RunAction, list[dict]]] +) -> bool: + """Whether `run`'s recorded actions may be applied without a human. + + The whole unattended-apply policy in one place, so "why didn't this apply?" has one + answer, and every gate fails closed. + + Judged on the persisted rows rather than `summary["actions"]`, because the rows are + what gets dispatched: `ensure_action_rows` never rewrites an existing row, so if the + two diverge, checking the summary would approve one payload while a different one + went to Bugzilla. + """ + if spec is None or not spec.auto_apply_actions: + return False + + # `rules/scoping.md` pairs an out-of-scope report with `confidence: low`, but nothing + # makes the agent do so — a `high` + `actionable: false` run would otherwise post an + # out-of-scope note on the strength of the confidence alone. `is False`, so a missing + # `actionable` doesn't read as "out of scope". + findings = (run.summary or {}).get("findings") or {} + if findings.get("actionable") is False: + return False + + if ( + spec.auto_apply_confidence is not None + and _reported_confidence(run) not in spec.auto_apply_confidence + ): + return False + + if spec.auto_apply_guard is not None: + reason = spec.auto_apply_guard(run, [row for row, _ in rows]) + if reason is not None: + log.warning( + "Holding run %s for review: %s (agent %s)", + run.run_id, + reason, + run.agent, + ) + return False + + return True + + + async def ensure_action_rows( db: AsyncSession, run: Run ) -> list[tuple[RunAction, list[dict]]]: @@ -224,11 +276,11 @@ async def _apply_pending_rows( async def on_run_completed(db: AsyncSession, run: Run) -> None: - """Record a completed run's actions, and auto-apply them if the agent opts in. + """Record a completed run's actions, and auto-apply them if the agent qualifies. - Called from the `apply-run-actions` push route. Actions are always recorded - (so the UI can show/manually apply them); they're applied automatically only - when the run's agent has `auto_apply_actions=True`. + Called from the `apply-run-actions` push route. Actions are always recorded (so the + UI can show/manually apply them); they're applied automatically only when + `_should_auto_apply` says so. """ # Defense-in-depth: only a succeeded run's actions are recorded/applied. A # failed/timed-out run may have recorded actions before erroring, but acting @@ -243,15 +295,17 @@ async def on_run_completed(db: AsyncSession, run: Run) -> None: await db.commit() spec = AGENT_REGISTRY.get(run.agent) - if spec and spec.auto_apply_actions: + if _should_auto_apply(spec, run, rows): await _apply_pending_rows(db, run, rows) - else: - log.info( - "Recorded %d action(s) for run %s; auto-apply off for agent %s", - len(rows), - run.run_id, - run.agent, - ) + return + + log.info( + "Recorded %d action(s) for run %s; not auto-applying (agent %s, confidence %s)", + len(rows), + run.run_id, + run.agent, + _reported_confidence(run), + ) async def apply_all_pending(db: AsyncSession, run: Run) -> None: diff --git a/services/hackbot-api/app/agents.py b/services/hackbot-api/app/agents.py index e9daa4dcf1..cfd42ec48b 100644 --- a/services/hackbot-api/app/agents.py +++ b/services/hackbot-api/app/agents.py @@ -4,10 +4,13 @@ from pydantic import BaseModel +from app.auto_apply import frontend_triage_guard +from app.database.models import Run, RunAction from app.schemas import ( AutowebcompatReproInputs, BugFixInputs, BuildRepairInputs, + Confidence, FrontendTriageInputs, TestPlanGeneratorInputs, TestRepairInputs, @@ -27,6 +30,12 @@ class AgentSpec: # succeeds. Off by default: actions are still recorded and can always be # applied manually from the UI; only opted-in agents auto-apply. auto_apply_actions: bool = False + # Fail closed: when set, a run whose findings carry no usable confidence never + # qualifies. Widening the policy is an edit to this set, not to the applier. + auto_apply_confidence: frozenset[Confidence] | None = None + # Bounds what a run may write unattended, beyond what confidence already gates. + # Returns the reason a human is needed, or None. See app/auto_apply.py. + auto_apply_guard: Callable[[Run, list[RunAction]], str | None] | None = None def model_to_env(inputs: BaseModel) -> dict[str, str]: @@ -80,6 +89,11 @@ def model_to_env(inputs: BaseModel) -> dict[str, str]: description="Triage a Firefox desktop frontend bug (read-only) and produce a root-cause analysis and proposed fix plan.", job_name="hackbot-agent-frontend-triage", input_schema=FrontendTriageInputs, + # Triage results reach a real bug unattended, so only the ones the agent + # localized to specific code qualify. Medium/low stays for manual apply. + auto_apply_actions=True, + auto_apply_confidence=frozenset({Confidence.high}), + auto_apply_guard=frontend_triage_guard, ), "test-repair": AgentSpec( name="test-repair", diff --git a/services/hackbot-api/app/auto_apply.py b/services/hackbot-api/app/auto_apply.py new file mode 100644 index 0000000000..4de26ddff2 --- /dev/null +++ b/services/hackbot-api/app/auto_apply.py @@ -0,0 +1,111 @@ +"""Per-agent limits on what a run may write to Bugzilla without a human. + +An agent's `confidence` gates its *judgement*; a guard here gates its *reach*. The +two are separate because an action's params are model output and the apply step +dispatches them against the runtime's global handler registry — which can create +bugs, attach files and write to Phabricator — so restricting which tools the agent +was given does not restrict what its recorded actions can reach. +""" + +from __future__ import annotations + +from typing import Any + +from app.database.models import Run, RunAction + +_TRIAGE_FIELDS = frozenset({"keywords", "severity"}) + + +def _is_bug_id(value: Any) -> bool: + # An int or a plain run of digits, nothing looser: the handler interpolates this + # raw value into the REST path, whereas `int()` would also accept `"2_014_702"`, + # signs, whitespace and non-ASCII digits — validating a different string than the + # one sent. + if isinstance(value, bool): + return False + return isinstance(value, int) or (isinstance(value, str) and value.isdigit()) + + +def _field_change(field: str, value: Any) -> str | None: + """Why setting `field` to `value` is more than an addition, or None.""" + if field == "severity": + # A single-valued field has no additive form, so a scalar is the only way to + # set it. The value isn't checked against Bugzilla's vocabulary: an unknown one + # is rejected there, surfacing as a failed action rather than a wrong write. + if isinstance(value, str) and value.strip(): + return None + return f"severity is set to an unexpected {type(value).__name__}" + + # A bare list *replaces* every keyword already on the bug; `{"add": [...]}` is the + # only form that adds. + if not isinstance(value, dict): + return f"{field} is set wholesale rather than added to" + if set(value) - {"add"}: + return f"{field} is edited with {', '.join(sorted(value))}, not add" + additions = value.get("add") + if not isinstance(additions, list) or not additions: + return f"{field}'s add is not a non-empty list" + if not all(isinstance(item, str) and item.strip() for item in additions): + return f"{field} adds something that isn't a non-empty string" + return None + + +def frontend_triage_guard(run: Run, rows: list[RunAction]) -> str | None: + """Why this triage run needs a human, or None if it may apply unattended. + + What `rules/frontend-triage.md` sanctions: one plan comment and, at most, one + obviously-correct field addition on the bug the run was asked about. A run + proposing anything else is held whole rather than part-applied — the comment + explains the field change and the two are coalesced into one Bugzilla PUT, so + dropping one and applying the rest would post something the agent didn't propose. + """ + expected_bug_id = (run.inputs or {}).get("bug_id") + seen: set[str] = set() + + for row in rows: + params = row.params or {} + + if row.type not in ("bugzilla.add_comment", "bugzilla.update_bug"): + return f"{row.type} is not an action type it may apply unattended" + if row.type in seen: + return f"it records more than one {row.type}" + seen.add(row.type) + + bug_id = params.get("bug_id") + # Required, not merely compared when present: `bugzilla.create_bug` carries no + # `bug_id` at all, and "no target" must not read as "target matches". + if bug_id is None or expected_bug_id is None: + return f"{row.type} names no bug to check against the run's input" + if not _is_bug_id(bug_id): + return f"it targets an unreadable bug id {bug_id!r}" + if int(bug_id) != int(expected_bug_id): + return f"it targets bug {bug_id}, not the run's bug {expected_bug_id}" + + # A private comment is invisible to the reporter and the public, which defeats + # the review-by-visibility this design leans on. Wanting privacy is exactly the + # case that wants a human. + if row.type == "bugzilla.add_comment" and params.get("is_private"): + return "it posts a private comment" + + if row.type != "bugzilla.update_bug": + continue + + # `UpdateBugHandler` forwards a `comment` param straight into the PUT, so it is a + # second route to posting one — including a private one, past the check above. + # (A `comment` key inside `changes` is a third, caught by the allowlist below.) + if params.get("comment") is not None: + return "it carries its own comment rather than a coalesced one" + # Not `or {}`: a falsey non-mapping (`[]`, `""`, `0`) would become an empty dict + # and sail through as "changes nothing" instead of being held. + changes = params.get("changes") + if not isinstance(changes, dict) or not changes: + return "its `changes` is not a non-empty mapping of fields" + disallowed = sorted(set(changes) - _TRIAGE_FIELDS) + if disallowed: + return f"it changes {', '.join(disallowed)}, which it may not change" + for field, value in changes.items(): + reason = _field_change(field, value) + if reason is not None: + return reason + + return None diff --git a/services/hackbot-api/app/schemas.py b/services/hackbot-api/app/schemas.py index 0796248dd8..867a6c5b63 100644 --- a/services/hackbot-api/app/schemas.py +++ b/services/hackbot-api/app/schemas.py @@ -14,6 +14,33 @@ class RunStatus(str, Enum): timed_out = "timed_out" +class Confidence(str, Enum): + """How sure an agent is of its own findings. + + Reported by the agent inside a free-form JSON block, so it arrives as untrusted + text — use `parse_confidence` rather than the constructor. + """ + + high = "high" + medium = "medium" + low = "low" + + +def parse_confidence(value: object) -> Confidence | None: + """`value` as a `Confidence`, or None if it isn't one. + + Tolerates casing and stray whitespace, because the value is parsed out of + model output and "High" must not silently read as "no confidence". Anything + else is None: callers fail closed rather than inventing a level. + """ + if not isinstance(value, str): + return None + try: + return Confidence(value.strip().lower()) + except ValueError: + return None + + class ArtifactRef(BaseModel): name: str size: int diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index 344c41fd59..767c037a2b 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -7,7 +7,7 @@ import logging import uuid -from dataclasses import dataclass, field +from dataclasses import dataclass, field, replace from types import SimpleNamespace from app import actions_applier @@ -16,7 +16,9 @@ on_run_completed, resolve_placeholders, ) -from app.schemas import RunStatus +from app.agents import AGENT_REGISTRY +from app.auto_apply import frontend_triage_guard +from app.schemas import Confidence, RunStatus def test_resolves_known_ref_and_field(): @@ -75,6 +77,391 @@ class _FakeRun: agent: str = "bug-fix" run_id: uuid.UUID = field(default_factory=uuid.uuid4) summary: dict | None = None + inputs: dict = field(default_factory=dict) + + +def _spec(*, auto=True, confidence=None, guard=None): + """A real `AgentSpec`, not a stand-in. + + Built with `replace` off a registry entry so every field defaults to the production + default and a newly-added one can't silently read as absent — a hand-rolled + namespace would raise `AttributeError` deep inside the gate instead of exercising it. + """ + return replace( + AGENT_REGISTRY["bug-fix"], + auto_apply_actions=auto, + auto_apply_confidence=confidence, + auto_apply_guard=guard, + ) + + +def _rows_for(run): + """The `(row, attachments)` pairs `ensure_action_rows` would produce for `run`. + + The gate judges the persisted rows, since those are what gets dispatched. + """ + return [ + ( + _row( + idx, + "pending", + action_type=action.get("type"), + params=action.get("params") or {}, + ), + [], + ) + for idx, action in enumerate((run.summary or {}).get("actions", [])) + ] + + +def _auto_applies(spec, run): + return actions_applier._should_auto_apply(spec, run, _rows_for(run)) + + +def _run_with_confidence(confidence): + return _FakeRun( + status=RunStatus.succeeded.value, + summary={"findings": {"confidence": confidence}, "actions": []}, + ) + + +def test_auto_apply_off_never_applies(): + # The master switch wins: confidence is irrelevant when the agent hasn't + # opted in at all. + spec = _spec(auto=False, confidence=frozenset({Confidence.high})) + assert not _auto_applies(spec, _run_with_confidence("high")) + + +def test_unknown_agent_never_applies(): + assert not _auto_applies(None, _run_with_confidence("high")) + + +def test_no_confidence_restriction_applies_unconditionally(): + # Existing/future agents that opt in without naming confidence levels keep + # applying regardless of what findings say. + spec = _spec(confidence=None) + assert _auto_applies(spec, _run_with_confidence("low")) + assert _auto_applies(spec, _FakeRun(status=RunStatus.succeeded.value)) + + +def test_confidence_restriction_admits_listed_level_only(): + spec = _spec(confidence=frozenset({Confidence.high})) + assert _auto_applies(spec, _run_with_confidence("high")) + assert not _auto_applies(spec, _run_with_confidence("medium")) + assert not _auto_applies(spec, _run_with_confidence("low")) + + +def test_confidence_restriction_can_widen_to_several_levels(): + # Widening to medium later must be a config change, not a code change. + spec = _spec(confidence=frozenset({Confidence.high, Confidence.medium})) + assert _auto_applies(spec, _run_with_confidence("medium")) + assert not _auto_applies(spec, _run_with_confidence("low")) + + +def test_confidence_restriction_is_case_and_space_insensitive(): + # `confidence` is parsed out of the model's free-form JSON block, so don't + # let "High" silently mean "never apply". + spec = _spec(confidence=frozenset({Confidence.high})) + assert _auto_applies(spec, _run_with_confidence("High")) + assert _auto_applies(spec, _run_with_confidence(" HIGH ")) + + +def test_specs_name_confidence_levels_that_runs_can_actually_report(): + # The same trap on the config side: a spec naming a level no run can ever + # report (`"High"`, `"very-high"`) would silently disable auto-apply. Both + # sides now speak `Confidence`, so it can't be written down — this asserts the + # registry actually uses it rather than raw strings that happen to match. + for name, spec in AGENT_REGISTRY.items(): + if spec.auto_apply_confidence is None: + continue + assert all( + isinstance(level, Confidence) for level in spec.auto_apply_confidence + ), name + + +def test_missing_confidence_fails_closed(): + spec = _spec(confidence=frozenset({Confidence.high})) + for run in ( + _FakeRun(status=RunStatus.succeeded.value), # no summary at all + _FakeRun(status=RunStatus.succeeded.value, summary={}), # no findings + _FakeRun(status=RunStatus.succeeded.value, summary={"findings": {}}), + _run_with_confidence(None), + _run_with_confidence(""), + _run_with_confidence(42), # not even a string + ): + assert not _auto_applies(spec, run) + + +# --- what an agent may do unattended ------------------------------------ # +# +# `confidence` gates the agent's judgement; these gate its reach. Action params +# are model output, and the agent spends the run reading bug comments nobody +# controls, so a high-confidence run is not by itself a licence to write anything +# anywhere. + + +TRIAGE_FIELDS = frozenset({"keywords", "severity"}) + + +def _run_with_actions(*actions, confidence="high", bug_id=2014702, **findings): + base = {"confidence": confidence, **findings} + return _FakeRun( + status=RunStatus.succeeded.value, + inputs={"bug_id": bug_id}, + summary={"findings": base, "actions": list(actions)}, + ) + + +def _comment(bug_id=2014702): + return {"type": "bugzilla.add_comment", "params": {"bug_id": bug_id, "text": "hi"}} + + +def _update(changes, bug_id=2014702): + return { + "type": "bugzilla.update_bug", + "params": {"bug_id": bug_id, "changes": changes}, + } + + +TRIAGE_TYPES = frozenset({"bugzilla.add_comment", "bugzilla.update_bug"}) + + +def _triage_spec(): + # The real guard, so these exercise the shipped policy rather than a paraphrase. + return _spec(confidence=frozenset({Confidence.high}), guard=frontend_triage_guard) + + +def test_actions_within_the_agents_authority_are_applied(): + run = _run_with_actions(_comment(), _update({"keywords": {"add": ["perf"]}})) + assert _auto_applies(_triage_spec(), run) + + +def test_an_action_against_another_bug_holds_the_run(): + # The run was asked about one bug; it may not write to a different one. + run = _run_with_actions(_comment(bug_id=999), bug_id=2014702) + assert not _auto_applies(_triage_spec(), run) + + +def test_a_field_outside_the_allowlist_holds_the_run(): + # `bugzilla.update_bug` accepts any field the REST endpoint does. Triage + # diagnoses; it does not resolve or reassign. + for changes in ( + {"status": "RESOLVED"}, + {"resolution": "DUPLICATE"}, + {"assigned_to": "someone@mozilla.com"}, + {"component": "General"}, + {"keywords": {"add": ["perf"]}, "status": "RESOLVED"}, # one bad key is enough + ): + run = _run_with_actions(_update(changes)) + assert not _auto_applies(_triage_spec(), run), changes + + +def test_one_out_of_bounds_action_holds_the_whole_run(): + # All-or-nothing: the comment explains the field change and the two are + # coalesced into a single Bugzilla PUT, so applying half would post something + # the agent never proposed. + run = _run_with_actions(_comment(), _update({"status": "RESOLVED"})) + assert not _auto_applies(_triage_spec(), run) + + +def test_a_destructive_keyword_edit_holds_the_run(): + # The allowlist names `keywords`, but naming a field is not permitting every + # edit to it. Bugzilla list fields take add/remove/set, and the recording tool + # advertises all three to the model — so a name-only check would let a bug + # comment talk the agent into wiping the bug's keywords unattended. + for changes in ( + {"keywords": {"set": []}}, # wipes every keyword + {"keywords": {"set": ["perf"]}}, # replaces rather than adds + {"keywords": {"remove": ["regression"]}}, + {"keywords": {"add": ["perf"], "remove": ["regression"]}}, + {"keywords": {"add": "perf"}}, # not a list + ): + run = _run_with_actions(_update(changes)) + assert not _auto_applies(_triage_spec(), run), changes + + +def test_adding_a_keyword_is_still_allowed(): + run = _run_with_actions(_update({"keywords": {"add": ["perf"]}})) + assert _auto_applies(_triage_spec(), run) + + +def test_a_nonsense_change_value_holds_the_run(): + for changes in ( + {"keywords": {"add": []}}, # adds nothing + {"keywords": {"add": [""]}}, # blank keyword + {"keywords": {"add": [" "]}}, + {"keywords": {"add": [{"unexpected": "structure"}]}}, + {"keywords": {"add": ["perf", 7]}}, + {"severity": ""}, # blank + {"severity": None}, + {"severity": True}, # a bool is not a severity + {"severity": ["S2"]}, + ): + run = _run_with_actions(_update(changes)) + assert not _auto_applies(_triage_spec(), run), changes + + +def test_changes_must_be_a_non_empty_mapping(): + # `params.get("changes") or {}` would turn the falsey ones into "changes nothing" + # and wave them through instead of holding them. + for changes in ([], "", 0, {}, "keywords=perf"): + run = _run_with_actions(_update(changes)) + assert not _auto_applies(_triage_spec(), run), repr(changes) + + +def test_a_plain_field_value_is_still_allowed(): + run = _run_with_actions(_update({"severity": "S2"})) + assert _auto_applies(_triage_spec(), run) + + +def test_a_private_comment_holds_the_run(): + # A private comment is invisible to the reporter and the public, so it defeats + # the review-by-visibility the unattended path leans on, and hides what the bot + # did. Wanting privacy is exactly the case that wants a human. + action = _comment() + action["params"]["is_private"] = True + assert not _auto_applies(_triage_spec(), _run_with_actions(action)) + + +def test_a_comment_smuggled_through_changes_holds_the_run(): + # `UpdateBugHandler` copies every `changes` key into the PUT body, so this is another + # route to posting one. The field allowlist is what catches it. + assert not _auto_applies( + _triage_spec(), _run_with_actions(_update({"comment": "hi"})) + ) + + +def test_an_update_carrying_its_own_comment_holds_the_run(): + # `UpdateBugHandler` forwards a `comment` param straight into the PUT, so this is + # a second route to posting one — including a private one, past the check above. + # The coalescer synthesises that field from the sibling row, so a recorded one is + # a payload nobody planned for. + action = _update({"keywords": {"add": ["perf"]}}) + action["params"]["comment"] = {"body": "hi", "is_private": True} + assert not _auto_applies(_triage_spec(), _run_with_actions(action)) + + +def test_setting_a_list_field_wholesale_holds_the_run(): + # The bare-scalar form replaces the field. For `severity` that's the only way to + # set it; for a list field like `keywords` it's the very replacement the add-only + # rule exists to prevent, so the scalar form is allowed only where it's safe. + assert not _auto_applies( + _triage_spec(), _run_with_actions(_update({"keywords": "regression"})) + ) + assert _auto_applies(_triage_spec(), _run_with_actions(_update({"severity": "S2"}))) + + +def test_the_gate_judges_the_rows_that_will_be_applied(monkeypatch): + # `ensure_action_rows` never rewrites an existing row, so a summary and its rows + # can diverge. Approving the summary while a different payload goes to Bugzilla + # would make the whole gate decorative. + run = _run_with_actions(_comment()) # summary looks harmless + rows = [ + ( + _row( + 0, + "pending", + action_type="bugzilla.update_bug", + params={"bug_id": 2014702, "changes": {"status": "RESOLVED"}}, + ), + [], + ) + ] + assert not actions_applier._should_auto_apply(_triage_spec(), run, rows) + + +def test_more_than_one_action_of_a_kind_holds_the_run(): + # An injected model told to write "a single brief comment" could record fifty. + run = _run_with_actions(_comment(), _comment(), _comment()) + assert not _auto_applies(_triage_spec(), run) + run = _run_with_actions(_update({"severity": "S2"}), _update({"severity": "S3"})) + assert not _auto_applies(_triage_spec(), run) + + +def test_an_agent_with_no_guard_is_unrestricted(): + # Agents that predate the guard keep their existing reach. + run = _run_with_actions(_update({"status": "RESOLVED"})) + assert _auto_applies(_spec(confidence=frozenset({Confidence.high})), run) + + +def test_an_action_type_the_agent_may_not_take_holds_the_run(): + # The applier dispatches against the runtime's *global* handler registry, so + # limiting the agent's tools does not limit what its persisted actions reach. + for action_type in ( + "bugzilla.add_attachment", + "phabricator.submit_patch", + "phabricator.add_comment", + "testrail.submit_test_plan", + ): + run = _run_with_actions( + {"type": action_type, "params": {"bug_id": 2014702}}, + ) + assert not _auto_applies(_triage_spec(), run), action_type + + +def test_a_create_bug_action_holds_the_run(): + # `bugzilla.create_bug` has no `bug_id` at all, so a same-bug check that only + # fires "when present" would wave it through and file a brand new bug. + run = _run_with_actions({"type": "bugzilla.create_bug", "params": {"summary": "x"}}) + assert not _auto_applies(_triage_spec(), run) + + +def test_an_action_with_no_bug_id_holds_a_bug_scoped_run(): + run = _run_with_actions({"type": "bugzilla.add_comment", "params": {"text": "hi"}}) + assert not _auto_applies(_triage_spec(), run) + + +def test_a_bug_scoped_run_with_no_bug_input_is_held(): + # Nothing trustworthy to compare the action against. + run = _FakeRun( + status=RunStatus.succeeded.value, + inputs={}, + summary={"findings": {"confidence": "high"}, "actions": [_comment()]}, + ) + assert not _auto_applies(_triage_spec(), run) + + +def test_the_real_frontend_triage_spec_is_guarded(): + # The live wiring, not a paraphrase of it: without this the whole guard is + # unreachable and every rule above tests a spec nothing uses. + assert AGENT_REGISTRY["frontend-triage"].auto_apply_guard is frontend_triage_guard + + +def test_only_bug_fix_still_auto_applies_without_a_guard(): + # A tripwire, and a record of a known gap rather than an endorsement of it. + # + # `bug-fix` auto-applies unguarded, so a succeeded run of it can dispatch anything + # in the runtime's global handler registry — creating bugs, attaching files, + # submitting Phabricator patches — against any target the model names. That + # predates this change and bounding it is a product decision about that agent. This + # assertion exists so the next agent to opt in has to guard itself or say why not. + unguarded = { + name + for name, spec in AGENT_REGISTRY.items() + if spec.auto_apply_actions and spec.auto_apply_guard is None + } + assert unguarded == {"bug-fix"} + + +def test_an_out_of_scope_run_is_held_even_at_high_confidence(): + # `rules/scoping.md` pairs out-of-scope with `actionable: false` *and* + # `confidence: low`, but nothing makes the agent pair them. + run = _run_with_actions(_comment(), actionable=False) + assert not _auto_applies(_triage_spec(), run) + # An agent that simply doesn't report `actionable` is not held back by it. + assert _auto_applies(_triage_spec(), _run_with_actions(_comment())) + assert _auto_applies(_triage_spec(), _run_with_actions(_comment(), actionable=True)) + + +def test_a_bug_id_that_is_not_a_number_holds_the_run(): + run = _run_with_actions(_comment(bug_id="not-a-bug")) + assert not _auto_applies(_triage_spec(), run) + + +def test_a_bug_id_given_as_a_string_still_matches(): + # JSON round-trips can stringify it; that's not an authority violation. + run = _run_with_actions(_comment(bug_id="2014702"), bug_id=2014702) + assert _auto_applies(_triage_spec(), run) class _FakeDB: @@ -90,7 +477,7 @@ async def execute(self, *a, **k): ) -def _patch_applier(monkeypatch, *, auto: bool | None): +def _patch_applier(monkeypatch, *, auto: bool | None, confidence=None): """Stub ensure/apply and the registry; record what got called. `auto=None` means the agent isn't in the registry at all. @@ -107,7 +494,7 @@ async def fake_apply(db, run, rows): monkeypatch.setattr(actions_applier, "ensure_action_rows", fake_ensure) monkeypatch.setattr(actions_applier, "_apply_pending_rows", fake_apply) registry = ( - {} if auto is None else {"bug-fix": SimpleNamespace(auto_apply_actions=auto)} + {} if auto is None else {"bug-fix": _spec(auto=auto, confidence=confidence)} ) monkeypatch.setattr(actions_applier, "AGENT_REGISTRY", registry) return calls @@ -142,6 +529,45 @@ async def test_succeeded_unknown_agent_does_not_apply(monkeypatch): assert calls == {"ensured": True, "applied": False} +async def test_succeeded_high_confidence_run_applies(monkeypatch): + calls = _patch_applier( + monkeypatch, auto=True, confidence=frozenset({Confidence.high}) + ) + await on_run_completed(_FakeDB(), _run_with_confidence("high")) + assert calls == {"ensured": True, "applied": True} + + +async def test_succeeded_low_confidence_run_records_but_does_not_apply(monkeypatch): + calls = _patch_applier( + monkeypatch, auto=True, confidence=frozenset({Confidence.high}) + ) + for level in ("medium", "low"): + calls["ensured"] = calls["applied"] = False + await on_run_completed(_FakeDB(), _run_with_confidence(level)) + # Recorded for the UI (and manual apply), but nothing reaches Bugzilla. + assert calls == {"ensured": True, "applied": False}, level + + +async def test_frontend_triage_auto_applies_at_high_confidence_only(): + # Guards the live policy, not the mechanism: widening this set posts + # lower-confidence analyses to real bugs, so it should be a deliberate edit. + spec = AGENT_REGISTRY["frontend-triage"] + assert spec.auto_apply_actions is True + assert spec.auto_apply_confidence == frozenset({Confidence.high}) + + +async def test_other_agents_do_not_auto_apply(): + # Opting an agent in is a deliberate edit, so spell out who is in today: + # bug-fix auto-applies unconditionally, frontend-triage only at high + # confidence, and everyone else stays human-gated. + auto_apply = {n for n, s in AGENT_REGISTRY.items() if s.auto_apply_actions} + assert auto_apply == {"bug-fix", "frontend-triage"} + + # frontend-triage is still the only agent with a confidence gate. + gated = {n for n, s in AGENT_REGISTRY.items() if s.auto_apply_confidence} + assert gated == {"frontend-triage"} + + async def test_apply_all_pending_always_applies(monkeypatch): # Manual apply ignores the opt-in flag entirely. calls = _patch_applier(monkeypatch, auto=False)