diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index bdb95ca17a..c51c5c08b2 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/services/hackbot-api/app/actions_applier.py @@ -3,11 +3,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* is decided by -`_auto_apply_blocker` (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. +the agent's run-level policy and per-action overrides (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 @@ -108,6 +109,17 @@ def _auto_apply_blocker(spec: AgentSpec | None, run: Run) -> str | None: return None +def _should_auto_apply( + spec: AgentSpec, action_type: str, *, run_level_auto_apply: bool +) -> bool: + """Apply per-action overrides, falling back to the run-level decision.""" + if action_type in spec.never_apply_actions: + return False + if action_type in spec.always_apply_actions: + return True + return run_level_auto_apply + + async def ensure_action_rows( db: AsyncSession, run: Run ) -> list[tuple[RunAction, list[dict]]]: @@ -247,11 +259,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 qualifies. + """Record a completed run's actions and auto-apply each eligible action. 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 - `_auto_apply_blocker` finds nothing in the way. + UI can show/manually apply them); agent defaults and per-action overrides decide + which ones are applied automatically. """ # 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 @@ -266,18 +278,28 @@ async def on_run_completed(db: AsyncSession, run: Run) -> None: await db.commit() spec = AGENT_REGISTRY.get(run.agent) - blocker = _auto_apply_blocker(spec, run) - if blocker is None: - await _apply_pending_rows(db, run, rows) - return - log.info( - "Recorded %d action(s) for run %s; holding for review: %s (agent %s)", - len(rows), - run.run_id, - blocker, - run.agent, - ) + run_level_auto_apply = _auto_apply_blocker(spec, run) is None + to_apply: list[tuple[RunAction, list[dict]]] = [] + for row_with_attachments in rows: + row, _ = row_with_attachments + if _should_auto_apply( + spec, row.type, run_level_auto_apply=run_level_auto_apply + ): + to_apply.append(row_with_attachments) + + if to_apply: + await _apply_pending_rows(db, run, to_apply) + + held_count = len(rows) - len(to_apply) + if held_count: + log.info( + "Recorded %d action(s) for run %s; holding %d for review (agent %s)", + len(rows), + run.run_id, + held_count, + run.agent, + ) 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 0876868949..4f2b7499af 100644 --- a/services/hackbot-api/app/agents.py +++ b/services/hackbot-api/app/agents.py @@ -34,6 +34,9 @@ class AgentSpec: # was; this is where that verdict is honored. Fails closed, so a run that reports # no verdict never qualifies. auto_apply_requires_consent: bool = False + # Per-action overrides for the agent-level auto-apply policy. + always_apply_actions: frozenset[str] = frozenset() + never_apply_actions: frozenset[str] = frozenset() def model_to_env(inputs: BaseModel) -> dict[str, str]: diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index 8e944df6dc..0bdc70fade 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/services/hackbot-api/tests/test_actions_applier.py @@ -79,7 +79,7 @@ class _FakeRun: inputs: dict = field(default_factory=dict) -def _spec(*, auto=True, consent=False): +def _spec(*, auto=True, consent=False, always=frozenset(), never=frozenset()): """A real `AgentSpec` built with `replace` off a registry entry. Every field then takes its production default, so a field added later can't read @@ -90,6 +90,8 @@ def _spec(*, auto=True, consent=False): AGENT_REGISTRY["bug-fix"], auto_apply_actions=auto, auto_apply_requires_consent=consent, + always_apply_actions=always, + never_apply_actions=never, ) @@ -97,6 +99,13 @@ def _auto_applies(spec, run): return actions_applier._auto_apply_blocker(spec, run) is None +def _action_auto_applies(spec, run, action_type): + run_level_auto_apply = actions_applier._auto_apply_blocker(spec, run) is None + return actions_applier._should_auto_apply( + spec, action_type, run_level_auto_apply=run_level_auto_apply + ) + + def _run_with_findings(**findings): return _FakeRun( status=RunStatus.succeeded.value, @@ -124,6 +133,32 @@ def test_an_agent_that_needs_no_consent_applies_unconditionally(): assert _auto_applies(spec, _run_with_findings(auto_apply=False)) +def test_always_apply_action_overrides_agent_default(): + action_type = "slack.post_message" + spec = _spec(auto=False, always=frozenset({action_type})) + assert _action_auto_applies(spec, _run_with_findings(), action_type) + + +def test_always_apply_action_overrides_missing_consent(): + action_type = "slack.post_message" + spec = _spec(auto=True, consent=True, always=frozenset({action_type})) + assert _action_auto_applies(spec, _run_with_findings(auto_apply=False), action_type) + + +def test_never_apply_action_overrides_agent_default(): + action_type = "slack.post_message" + spec = _spec(auto=True, never=frozenset({action_type})) + assert not _action_auto_applies(spec, _run_with_findings(), action_type) + + +def test_action_without_override_uses_agent_default(): + spec = _spec( + auto=False, + always=frozenset({"slack.post_message"}), + ) + assert not _action_auto_applies(spec, _run_with_findings(), "bugzilla.add_comment") + + # --- the run's own verdict ----------------------------------------------- # # # The agent decides `findings.auto_apply`, because `confidence` lands in its final @@ -204,7 +239,7 @@ def _patch_applier(monkeypatch, *, auto: bool | None, consent=False): async def fake_ensure(db, run): calls["ensured"] = True - return [("row", [])] + return [(SimpleNamespace(type="bugzilla.add_comment"), [])] async def fake_apply(db, run, rows): calls["applied"] = True @@ -258,6 +293,32 @@ async def test_succeeded_unvouched_run_records_but_does_not_apply(monkeypatch): assert calls == {"ensured": True, "applied": False} +async def test_succeeded_run_only_applies_eligible_action_types(monkeypatch): + rows = [ + (SimpleNamespace(type="bugzilla.update_bug"), []), + (SimpleNamespace(type="bugzilla.add_comment"), []), + ] + applied_types = [] + + async def fake_ensure(db, run): + return rows + + async def fake_apply(db, run, selected_rows): + applied_types.extend(row.type for row, _ in selected_rows) + + spec = _spec( + auto=True, + never=frozenset({"bugzilla.add_comment"}), + ) + monkeypatch.setattr(actions_applier, "ensure_action_rows", fake_ensure) + monkeypatch.setattr(actions_applier, "_apply_pending_rows", fake_apply) + monkeypatch.setattr(actions_applier, "AGENT_REGISTRY", {"bug-fix": spec}) + + await on_run_completed(_FakeDB(), _FakeRun(status=RunStatus.succeeded.value)) + + assert applied_types == ["bugzilla.update_bug"] + + 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 and test-repair auto-apply unconditionally, frontend-triage only when