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)