Add per-run review requirement for agent actions

docs/hackbot/actions.md

@@ -89,8 +89,9 @@ Triggered by the `run.completed` event, on a subscription filtered to **succeede
 1. **Record rows.** Every entry in `summary.json`'s `actions` is upserted as a
    `run_actions` row (`pending`), keyed `(run_id, idx)`. This happens for _all_ succeeded
    runs, whether or not the agent auto-applies, so the UI can always show and apply them.
-2. **Apply, if opted in.** With `auto_apply_actions=True` on the agent's registry entry,
-   pending rows are applied immediately. Otherwise they wait for a human to click apply.
+2. **Apply, if opted in.** `require_review=true` always leaves a run's actions pending
+   for manual approval. Otherwise, actions are applied only when the agent opts in with
+   `auto_apply_actions=True` and any agent-specific consent check passes.
 3. **Dispatch.** Each row's `type` selects a handler from the registry. The handler gets
    the params and an `ApplyContext` — which can `download_artifact(key)` without knowing
    GCS is behind it, keeping the runtime library free of a storage dependency.

docs/hackbot/agents.md

@@ -80,6 +80,8 @@ Two additions in [services/hackbot-api/](../../services/hackbot-api/):
    validates against, and what becomes the run's env overrides.
 2. **[app/agents.py](../../services/hackbot-api/app/agents.py)** — one `AGENT_REGISTRY` entry: `name`, `description`, `job_name`
    (the Cloud Run Job), `input_schema`, and optionally `auto_apply_actions=True`.
+   That flag only makes the agent eligible for automatic application;
+   `require_review=true` on a run still holds its actions for manual approval.
 
 Env vars are derived from the schema (`bug_id` → `BUG_ID`, lists and dicts JSON-encoded),
 so there is no per-agent mapping code to write. `build_env` exists as an escape hatch for

docs/hackbot/api.md

@@ -22,6 +22,12 @@ component that knows the agent catalog, and the only writer of run state.
 user's email, stored as `requested_by` — the caller is a trusted service (the UI), so this
 is attribution, not authentication.
 
+Every agent input schema includes `require_review`, which defaults to `false`. When set,
+the run's actions stay pending for manual approval; otherwise, the agent's existing
+auto-apply policy decides. The value is stored with the run inputs so it remains available
+at completion, but it is not forwarded to the agent environment or prompt. Retriggers
+preserve the value because they reuse the original inputs.
+
 Artifact downloads are restricted to artifacts already listed on the run, which both scopes
 the download to that run's prefix and prevents probing unrelated objects.
 

services/hackbot-api/app/actions_applier.py

@@ -86,7 +86,11 @@ def _sub(match: re.Match) -> str:
     return value
 
 
-def _auto_apply_blocker(spec: AgentSpec | None, run: Run) -> str | None:
+def _auto_apply_blocker(
+    spec: AgentSpec | None,
+    run: Run,
+    require_review: bool | None,
+) -> str | None:
     """Why `run`'s recorded actions need a human, or None if they may be applied.
 
     This holds no policy about what an agent may record. That is bounded on the agent
@@ -96,6 +100,10 @@ def _auto_apply_blocker(spec: AgentSpec | None, run: Run) -> str | None:
     sure it was; it reports that as `findings.auto_apply`. This function honors both
     and fails closed.
     """
+    # Only an explicit review request overrides the agent's policy.
+    if require_review is True:
+        return "this run was requested with review required"
+
     if spec is None or not spec.auto_apply_actions:
         return "auto-apply is off for this agent"
 
@@ -246,7 +254,11 @@ async def _apply_pending_rows(
                     results_by_ref[member.ref] = outcome.result
 
 
-async def on_run_completed(db: AsyncSession, run: Run) -> None:
+async def on_run_completed(
+    db: AsyncSession,
+    run: Run,
+    require_review: bool | None,
+) -> None:
     """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
@@ -266,7 +278,7 @@ 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)
+    blocker = _auto_apply_blocker(spec, run, require_review)
     if blocker is None:
         await _apply_pending_rows(db, run, rows)
         return

services/hackbot-api/app/agents.py

@@ -5,6 +5,7 @@
 from pydantic import BaseModel
 
 from app.schemas import (
+    AgentInputs,
     AutowebcompatDiagnosisInputs,
     AutowebcompatReproInputs,
     BugFixInputs,
@@ -14,6 +15,9 @@
     TestRepairInputs,
 )
 
+# Shared run fields are not forwarded to the agent environment.
+_PLATFORM_FIELDS = frozenset(AgentInputs.model_fields)
+
 
 @dataclass(frozen=True)
 class AgentSpec:
@@ -44,11 +48,11 @@ def model_to_env(inputs: BaseModel) -> dict[str, str]:
     ``pydantic_settings.BaseSettings`` (which upper-cases field names by
     default). Lists/dicts are JSON-encoded. Deploy-time constants (e.g. the
     broker loopback URL) are NOT inputs — they belong in the Job's static env
-    config, not here.
+    config, not here. Shared run fields are skipped.
     """
     env: dict[str, str] = {}
     for name, value in inputs.model_dump(mode="json").items():
-        if value is None:
+        if value is None or name in _PLATFORM_FIELDS:
             continue
         if isinstance(value, str):
             env[name.upper()] = value

services/hackbot-api/app/pubsub.py

@@ -67,7 +67,9 @@ async def _publish_event(
         log.exception("Failed to publish %s event", event_type)
 
 
-async def publish_run_completed(run_id: str, agent: str, status: str) -> None:
+async def publish_run_completed(
+    run_id: str, agent: str, status: str, require_review: bool | None
+) -> None:
     """Publish a ``run.completed`` event to the run-domain topic.
 
     Topics follow a per-domain convention, ``<domain>-events`` (the GCP project
@@ -83,6 +85,11 @@ async def publish_run_completed(run_id: str, agent: str, status: str) -> None:
     await _publish_event(
         settings.run_events_topic,
         event_type="run.completed",
-        payload={"run_id": run_id, "agent": agent, "status": status},
+        payload={
+            "run_id": run_id,
+            "agent": agent,
+            "status": status,
+            "require_review": require_review,
+        },
         attributes={"agent": agent, "status": status},
     )

services/hackbot-api/app/routers/events.py

@@ -134,4 +134,5 @@ async def apply_run_actions(
         log.warning("No run found for run_id %s", run_id)
         return
 
-    await on_run_completed(db, run)
+    # Forward the review choice to the action policy.
+    await on_run_completed(db, run, event.get("require_review"))

services/hackbot-api/app/routers/runs.py

@@ -281,7 +281,12 @@ async def finalize_run(db: AsyncSession, run: Run) -> None:
             run.run_id,
             run.agent,
         )
-    await pubsub.publish_run_completed(str(run.run_id), run.agent, run.status)
+    await pubsub.publish_run_completed(
+        str(run.run_id),
+        run.agent,
+        run.status,
+        run.inputs["require_review"],
+    )
 
 
 def _has_unsubmitted_patch(

services/hackbot-api/app/schemas.py

@@ -14,6 +14,12 @@ class RunStatus(str, Enum):
     timed_out = "timed_out"
 
 
+class AgentInputs(BaseModel):
+    """Run-level inputs shared by every agent."""
+
+    require_review: bool = False
+
+
 class ArtifactRef(BaseModel):
     name: str
     size: int
@@ -76,7 +82,7 @@ class RunDoc(BaseModel):
 # --- Per-agent input schemas ---
 
 
-class BugFixInputs(BaseModel):
+class BugFixInputs(AgentInputs):
     bug_id: int
     # When following up on an existing Phabricator revision (e.g. triggered by a
     # webhook), the revision to update and the comment that mentioned Hackbot.
@@ -110,7 +116,7 @@ def _validate_mode(self) -> "BugFixInputs":
         return self
 
 
-class AutowebcompatReproInputs(BaseModel):
+class AutowebcompatReproInputs(AgentInputs):
     bug_data: str | None = None
     bug_id: int | None = None
     model: str | None = None
@@ -124,7 +130,7 @@ def _require_subject(self) -> "AutowebcompatReproInputs":
         return self
 
 
-class AutowebcompatDiagnosisInputs(BaseModel):
+class AutowebcompatDiagnosisInputs(AgentInputs):
     bug_data: str | None = None
     bug_id: int | None = None
     model: str | None = None
@@ -138,7 +144,7 @@ def _require_subject(self) -> "AutowebcompatDiagnosisInputs":
         return self
 
 
-class BuildRepairInputs(BaseModel):
+class BuildRepairInputs(AgentInputs):
     # Failing Taskcluster build tasks {task_name: task_id}; the agent resolves the
     # push commits from them. git_commit / bug_id are optional overrides.
     failure_tasks: dict[str, str]
@@ -149,7 +155,7 @@ class BuildRepairInputs(BaseModel):
     max_turns: int | None = None
 
 
-class TestRepairInputs(BaseModel):
+class TestRepairInputs(AgentInputs):
     # Failing Taskcluster test tasks {task_name: task_id}. The agent resolves the
     # push, the last-green revision and the candidate commit range itself from the
     # task id (the listener only filters which failures are worth investigating).
@@ -158,14 +164,14 @@ class TestRepairInputs(BaseModel):
     max_turns: int | None = None
 
 
-class FrontendTriageInputs(BaseModel):
+class FrontendTriageInputs(AgentInputs):
     bug_id: int
     model: str | None = None
     max_turns: int | None = None
     effort: str | None = None
 
 
-class TestPlanGeneratorInputs(BaseModel):
+class TestPlanGeneratorInputs(AgentInputs):
     feature_name: str
     feature_description: str
     test_scope: str

services/hackbot-api/tests/test_actions_applier.py

@@ -93,8 +93,8 @@ def _spec(*, auto=True, consent=False):
     )
 
 
-def _auto_applies(spec, run):
-    return actions_applier._auto_apply_blocker(spec, run) is None
+def _auto_applies(spec, run, require_review=False):
+    return actions_applier._auto_apply_blocker(spec, run, require_review) is None
 
 
 def _run_with_findings(**findings):
@@ -124,6 +124,22 @@ def test_an_agent_that_needs_no_consent_applies_unconditionally():
     assert _auto_applies(spec, _run_with_findings(auto_apply=False))
 
 
+def test_review_request_holds_actions_even_when_agent_auto_applies():
+    run = _run_with_findings(auto_apply=True)
+    assert not _auto_applies(_spec(), run, True)
+
+
+def test_only_an_explicit_request_holds_actions():
+    run = _run_with_findings(auto_apply=True)
+    assert _auto_applies(_spec(), run, None)
+    assert _auto_applies(_spec(), run, "sometimes")
+
+
+def test_agent_policy_still_gates_a_run_with_no_flag():
+    run = _run_with_findings(auto_apply=True)
+    assert not _auto_applies(_spec(auto=False), run, None)
+
+
 # --- the run's own verdict ----------------------------------------------- #
 #
 # The agent decides `findings.auto_apply`, because `confidence` lands in its final
@@ -219,42 +235,42 @@ async def fake_apply(db, run, rows):
 async def test_non_succeeded_run_records_nothing(monkeypatch):
     calls = _patch_applier(monkeypatch, auto=True)
     for status in (RunStatus.failed.value, RunStatus.timed_out.value):
-        await on_run_completed(_FakeDB(), _FakeRun(status=status))
+        await on_run_completed(_FakeDB(), _FakeRun(status=status), False)
     assert calls == {"ensured": False, "applied": False}
 
 
 async def test_succeeded_opted_in_agent_records_and_applies(monkeypatch):
     calls = _patch_applier(monkeypatch, auto=True)
     db = _FakeDB()
-    await on_run_completed(db, _FakeRun(status=RunStatus.succeeded.value))
+    await on_run_completed(db, _FakeRun(status=RunStatus.succeeded.value), False)
     assert calls == {"ensured": True, "applied": True}
     assert db.commits >= 1
 
 
 async def test_succeeded_non_opted_agent_records_but_does_not_apply(monkeypatch):
     calls = _patch_applier(monkeypatch, auto=False)
     db = _FakeDB()
-    await on_run_completed(db, _FakeRun(status=RunStatus.succeeded.value))
+    await on_run_completed(db, _FakeRun(status=RunStatus.succeeded.value), False)
     assert calls == {"ensured": True, "applied": False}
     assert db.commits >= 1
 
 
 async def test_succeeded_unknown_agent_does_not_apply(monkeypatch):
     calls = _patch_applier(monkeypatch, auto=None)
-    await on_run_completed(_FakeDB(), _FakeRun(status=RunStatus.succeeded.value))
+    await on_run_completed(_FakeDB(), _FakeRun(status=RunStatus.succeeded.value), False)
     assert calls == {"ensured": True, "applied": False}
 
 
 async def test_succeeded_vouched_for_run_applies(monkeypatch):
     calls = _patch_applier(monkeypatch, auto=True, consent=True)
-    await on_run_completed(_FakeDB(), _run_with_findings(auto_apply=True))
+    await on_run_completed(_FakeDB(), _run_with_findings(auto_apply=True), False)
     assert calls == {"ensured": True, "applied": True}
 
 
 async def test_succeeded_unvouched_run_records_but_does_not_apply(monkeypatch):
     calls = _patch_applier(monkeypatch, auto=True, consent=True)
     # Recorded for the UI (and manual apply), but nothing reaches Bugzilla.
-    await on_run_completed(_FakeDB(), _run_with_findings(auto_apply=False))
+    await on_run_completed(_FakeDB(), _run_with_findings(auto_apply=False), False)
     assert calls == {"ensured": True, "applied": False}
 
 

services/hackbot-api/tests/test_events.py

@@ -6,7 +6,10 @@
 
 import base64
 import json
+import uuid
+from types import SimpleNamespace
 
+from app.routers import events
 from app.routers.events import (
     _decode_pubsub_push_body,
     _execution_name_from_completion_log,
@@ -73,3 +76,51 @@ def test_execution_name_falls_back_to_labels():
 def test_execution_name_missing():
     assert _execution_name_from_completion_log({"protoPayload": {}}) is None
     assert _execution_name_from_completion_log({}) is None
+
+
+class _Request:
+    def __init__(self, payload: dict):
+        self._body = _push_envelope(payload)
+
+    async def json(self):
+        return self._body
+
+
+class _DB:
+    def __init__(self, run):
+        self.run = run
+
+    async def get(self, _model, _run_id):
+        return self.run
+
+
+async def test_apply_actions_event_passes_review_flag(monkeypatch):
+    run_id = uuid.uuid4()
+    run = SimpleNamespace(run_id=run_id)
+    captured = []
+
+    async def fake_on_completed(db, completed_run, require_review):
+        captured.append((db, completed_run, require_review))
+
+    monkeypatch.setattr(events, "on_run_completed", fake_on_completed)
+    db = _DB(run)
+    await events.apply_run_actions(
+        _Request({"run_id": str(run_id), "require_review": True}),
+        db,
+    )
+
+    assert captured == [(db, run, True)]
+
+
+async def test_apply_actions_event_without_flag_passes_none(monkeypatch):
+    run_id = uuid.uuid4()
+    run = SimpleNamespace(run_id=run_id)
+    captured = []
+
+    async def fake_on_completed(_db, _run, require_review):
+        captured.append(require_review)
+
+    monkeypatch.setattr(events, "on_run_completed", fake_on_completed)
+    await events.apply_run_actions(_Request({"run_id": str(run_id)}), _DB(run))
+
+    assert captured == [None]