From 21a376972ea50206c5edcee803b14bc95fb880ed Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Thu, 27 Aug 2026 22:00:00 +0200 Subject: [PATCH 1/3] Add per-run review requirement for agent actions --- services/hackbot-api/app/actions_applier.py | 18 ++- services/hackbot-api/app/agents.py | 8 +- services/hackbot-api/app/pubsub.py | 11 +- services/hackbot-api/app/routers/events.py | 3 +- services/hackbot-api/app/routers/runs.py | 7 +- services/hackbot-api/app/schemas.py | 20 ++-- .../hackbot-api/tests/test_actions_applier.py | 32 +++-- services/hackbot-api/tests/test_events.py | 51 ++++++++ .../hackbot-api/tests/test_finalize_run.py | 9 +- services/hackbot-api/tests/test_pubsub.py | 8 +- .../hackbot-api/tests/test_require_review.py | 113 ++++++++++++++++++ services/hackbot-ui/app/globals.css | 16 +++ .../hackbot-ui/components/TriggerForm.tsx | 16 ++- 13 files changed, 281 insertions(+), 31 deletions(-) create mode 100644 services/hackbot-api/tests/test_require_review.py diff --git a/services/hackbot-api/app/actions_applier.py b/services/hackbot-api/app/actions_applier.py index bdb95ca17a..6a1b1ad2cc 100644 --- a/services/hackbot-api/app/actions_applier.py +++ b/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 diff --git a/services/hackbot-api/app/agents.py b/services/hackbot-api/app/agents.py index 0876868949..7182ecf4da 100644 --- a/services/hackbot-api/app/agents.py +++ b/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 diff --git a/services/hackbot-api/app/pubsub.py b/services/hackbot-api/app/pubsub.py index 17936be0d3..f73523f381 100644 --- a/services/hackbot-api/app/pubsub.py +++ b/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, ``-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}, ) diff --git a/services/hackbot-api/app/routers/events.py b/services/hackbot-api/app/routers/events.py index d46da7ab6d..65322740c3 100644 --- a/services/hackbot-api/app/routers/events.py +++ b/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")) diff --git a/services/hackbot-api/app/routers/runs.py b/services/hackbot-api/app/routers/runs.py index c50c278fa2..5c9c6930f1 100644 --- a/services/hackbot-api/app/routers/runs.py +++ b/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( diff --git a/services/hackbot-api/app/schemas.py b/services/hackbot-api/app/schemas.py index 49e48d48a9..2739fd43e4 100644 --- a/services/hackbot-api/app/schemas.py +++ b/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 diff --git a/services/hackbot-api/tests/test_actions_applier.py b/services/hackbot-api/tests/test_actions_applier.py index 8e944df6dc..88f8aeb0f4 100644 --- a/services/hackbot-api/tests/test_actions_applier.py +++ b/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,14 +235,14 @@ 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 @@ -234,27 +250,27 @@ async def test_succeeded_opted_in_agent_records_and_applies(monkeypatch): 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} diff --git a/services/hackbot-api/tests/test_events.py b/services/hackbot-api/tests/test_events.py index 214ca8e912..c17f371bad 100644 --- a/services/hackbot-api/tests/test_events.py +++ b/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] diff --git a/services/hackbot-api/tests/test_finalize_run.py b/services/hackbot-api/tests/test_finalize_run.py index cc0149b1ec..a7dd99cdca 100644 --- a/services/hackbot-api/tests/test_finalize_run.py +++ b/services/hackbot-api/tests/test_finalize_run.py @@ -23,6 +23,7 @@ class _FakeRun: agent: str = "bug-fix" status: str = RunStatus.pending.value execution_name: str | None = "projects/p/locations/l/jobs/j/executions/e" + inputs: dict = field(default_factory=lambda: {"require_review": False}) artifacts: list = field(default_factory=list) summary: dict | None = None error: str | None = None @@ -41,8 +42,8 @@ async def commit(self): def _no_publish(monkeypatch): published = [] - async def fake_publish(run_id, agent, status): - published.append((run_id, agent, status)) + async def fake_publish(run_id, agent, status, require_review): + published.append((run_id, agent, status, require_review)) monkeypatch.setattr(pubsub, "publish_run_completed", fake_publish) return published @@ -93,7 +94,9 @@ async def test_finalizes_succeeded_run(monkeypatch, _no_publish): assert run.status == RunStatus.succeeded.value assert run.finalized_at is not None assert run.artifacts == [{"name": "summary.json", "size": 10, "content_type": None}] - assert _no_publish == [(str(run.run_id), run.agent, RunStatus.succeeded.value)] + assert _no_publish == [ + (str(run.run_id), run.agent, RunStatus.succeeded.value, False) + ] @pytest.mark.parametrize( diff --git a/services/hackbot-api/tests/test_pubsub.py b/services/hackbot-api/tests/test_pubsub.py index 35f7fc3572..b8eed593e8 100644 --- a/services/hackbot-api/tests/test_pubsub.py +++ b/services/hackbot-api/tests/test_pubsub.py @@ -40,14 +40,16 @@ def fake_sync(topic, data, attributes): monkeypatch.setattr(pubsub, "_publish_sync", fake_sync) - await pubsub.publish_run_completed("run-1", "bug-fix", "failed") + await pubsub.publish_run_completed("run-1", "bug-fix", "failed", True) assert captured["topic"] == pubsub.settings.run_events_topic # The keys the applier subscription filter matches on must be present. assert captured["attributes"]["event_type"] == "run.completed" assert captured["attributes"]["status"] == "failed" assert captured["attributes"]["agent"] == "bug-fix" - assert json.loads(captured["data"])["run_id"] == "run-1" + payload = json.loads(captured["data"]) + assert payload["run_id"] == "run-1" + assert payload["require_review"] is True async def test_publish_failure_is_swallowed(monkeypatch): @@ -57,4 +59,4 @@ def boom(*a, **k): monkeypatch.setattr(pubsub, "_publish_sync", boom) # Best-effort: a publish failure must not propagate (run is already # finalized before this is called). - await pubsub.publish_run_completed("run-1", "bug-fix", "succeeded") + await pubsub.publish_run_completed("run-1", "bug-fix", "succeeded", "auto") diff --git a/services/hackbot-api/tests/test_require_review.py b/services/hackbot-api/tests/test_require_review.py new file mode 100644 index 0000000000..fce556d3eb --- /dev/null +++ b/services/hackbot-api/tests/test_require_review.py @@ -0,0 +1,113 @@ +"""Tests for the shared `require_review` run input.""" + +import pytest +from app import gcs, jobs +from app.agents import AGENT_REGISTRY, model_to_env +from app.routers.runs import create_run +from app.schemas import AgentInputs + + +class _FakeDB: + def __init__(self): + self.added = None + self.commits = 0 + + def add(self, value): + self.added = value + + async def flush(self): + pass + + async def commit(self): + self.commits += 1 + + +async def _create(monkeypatch, payload: dict, agent: str = "bug-fix"): + triggered = {} + + async def fake_policy(run_id): + return {"url": "https://upload.example/", "fields": {"key": "v"}} + + async def fake_trigger(job_name, env): + triggered["job_name"] = job_name + triggered["env"] = env + return "projects/p/locations/l/jobs/j/executions/e" + + monkeypatch.setattr(gcs, "run_prefix", lambda run_id: f"results/{run_id}/") + monkeypatch.setattr(gcs, "generate_results_policy", fake_policy) + monkeypatch.setattr(jobs, "trigger_execution", fake_trigger) + + db = _FakeDB() + await create_run(agent, payload, on_behalf_of=None, db=db) + return db.added, triggered + + +# --- what reaches the database ------------------------------------------- # + + +async def test_explicit_review_is_persisted(monkeypatch): + run, _ = await _create(monkeypatch, {"bug_id": 1889001, "require_review": True}) + assert run.inputs["require_review"] is True + + +async def test_explicit_false_is_persisted(monkeypatch): + run, _ = await _create(monkeypatch, {"bug_id": 1889001, "require_review": False}) + assert run.inputs["require_review"] is False + + +async def test_omitted_flag_defaults_to_false(monkeypatch): + run, _ = await _create(monkeypatch, {"bug_id": 1889001}) + assert run.inputs["require_review"] is False + + +async def test_flag_is_stored_alongside_the_agents_own_inputs(monkeypatch): + run, _ = await _create(monkeypatch, {"bug_id": 1889001, "require_review": True}) + assert run.inputs["bug_id"] == 1889001 + assert run.inputs["require_review"] is True + + +async def test_every_agent_accepts_and_persists_the_flag(monkeypatch): + for agent, payload in ( + ("bug-fix", {"bug_id": 1}), + ("autowebcompat-repro", {"bug_id": 1}), + ("build-repair", {"failure_tasks": {"t": "1"}}), + ): + for value in (True, False): + run, _ = await _create( + monkeypatch, {**payload, "require_review": value}, agent=agent + ) + assert run.inputs["require_review"] is value + + +# --- what the agent is allowed to see ------------------------------------ # + + +async def test_flag_never_reaches_the_container_env(monkeypatch): + _, triggered = await _create( + monkeypatch, {"bug_id": 1889001, "require_review": True} + ) + assert "REQUIRE_REVIEW" not in triggered["env"] + assert triggered["env"]["BUG_ID"] == "1889001" + + +def test_model_to_env_withholds_every_platform_field(): + for spec in AGENT_REGISTRY.values(): + env = model_to_env(spec.input_schema.model_construct(require_review=True)) + for field in AgentInputs.model_fields: + assert field.upper() not in env + + +# --- validation ---------------------------------------------------------- # + + +async def test_non_boolean_flag_is_rejected(monkeypatch): + with pytest.raises(Exception) as exc: + await _create(monkeypatch, {"bug_id": 1889001, "require_review": "sometimes"}) + assert getattr(exc.value, "status_code", None) == 422 + + +def test_flag_is_declared_once_on_the_shared_base(): + assert "require_review" in AgentInputs.model_fields + for spec in AGENT_REGISTRY.values(): + assert issubclass(spec.input_schema, AgentInputs) + assert "require_review" in spec.input_schema.model_fields diff --git a/services/hackbot-ui/app/globals.css b/services/hackbot-ui/app/globals.css index cca871a241..3896873fff 100644 --- a/services/hackbot-ui/app/globals.css +++ b/services/hackbot-ui/app/globals.css @@ -145,6 +145,22 @@ textarea:focus { border-color: var(--accent); } +/* Keep checkboxes inline with their labels. */ +input[type="checkbox"] { + width: auto; + padding: 0; + margin: 0; + vertical-align: middle; + accent-color: var(--accent); +} + +.checkbox-field label { + display: flex; + align-items: center; + gap: 8px; + cursor: pointer; +} + .field { margin-bottom: 16px; } diff --git a/services/hackbot-ui/components/TriggerForm.tsx b/services/hackbot-ui/components/TriggerForm.tsx index e069c31aad..9cc0b82dcc 100644 --- a/services/hackbot-ui/components/TriggerForm.tsx +++ b/services/hackbot-ui/components/TriggerForm.tsx @@ -22,6 +22,9 @@ export function TriggerForm() { ); const [bugId, setBugId] = useState(() => params.get("bug_id") ?? ""); const [bugData, setBugData] = useState(() => params.get("bug_data") ?? ""); + const [requireReview, setRequireReview] = useState( + () => params.get("require_review") === "true" + ); const [gitCommit, setGitCommit] = useState( () => params.get("git_commit") ?? "" ); @@ -56,7 +59,7 @@ export function TriggerForm() { e.preventDefault(); setError(null); - const inputs: Record = {}; + const inputs: Record = { require_review: requireReview }; const parsedBugId = parseBugId(bugId); const hasBugId = parsedBugId !== null; @@ -356,6 +359,17 @@ export function TriggerForm() { )} +
+ +
+ From 39e15010b8abc9a6e9201e94f8a7644889b66db4 Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Fri, 28 Aug 2026 11:02:05 +0200 Subject: [PATCH 2/3] update docs with the new review requirements --- docs/hackbot/actions.md | 10 ++++++++-- docs/hackbot/agents.md | 2 ++ docs/hackbot/api.md | 12 ++++++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/docs/hackbot/actions.md b/docs/hackbot/actions.md index 7e09eecdf8..a4ba3f6582 100644 --- a/docs/hackbot/actions.md +++ b/docs/hackbot/actions.md @@ -89,8 +89,14 @@ 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.** A run created with `require_review=true` always waits for a + human to click apply. Otherwise it may apply immediately, but only when the agent + also has `auto_apply_actions=True`; agent-specific consent checks still apply. + Omitting the flag defaults to `false`. The choice is stored with the run's other + inputs and read back at completion onto the `run.completed` event; `model_to_env` + withholds it from the container env, so the agent never sees it. Only an explicit + `true` holds; anything else defers to the agent's own policy, which must still opt + in through `auto_apply_actions` before anything is applied. 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. diff --git a/docs/hackbot/agents.md b/docs/hackbot/agents.md index 4ac09aa8f9..0ffbe6465b 100644 --- a/docs/hackbot/agents.md +++ b/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 makes the agent eligible for automatic application; an individual + run created in `review` mode still holds its actions for a human. 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 diff --git a/docs/hackbot/api.md b/docs/hackbot/api.md index 2a2244d1e7..ffbe001511 100644 --- a/docs/hackbot/api.md +++ b/docs/hackbot/api.md @@ -22,6 +22,18 @@ 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. +The body also accepts `require_review: true|false`, declared once on the shared +`AgentInputs` base that every agent's input schema inherits — so it validates with the +agent's own inputs and appears in each published `/agents` schema. When true, recorded +external actions always stay pending for a human. When false (the default), the run is +only _eligible_ for automatic application: the agent must still opt in through +`AgentSpec.auto_apply_actions`, and any agent-specific consent check still applies. The +flag is persisted with the run's other inputs (JSONB, so no migration) and read back by +`finalize_run` onto the completion event, which is how the choice survives into the later +completion request. `model_to_env` withholds it from the container env, so it never +reaches the agent or its prompt. Retriggers replay the run's inputs, so a run that asked +for review is retried under review. + 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. From 59772ac28969038b1fb47f19d28be0082aedaa1a Mon Sep 17 00:00:00 2001 From: ayoubdiourin7 Date: Fri, 28 Aug 2026 11:10:21 +0200 Subject: [PATCH 3/3] Clarify per-run review documentation --- docs/hackbot/actions.md | 11 +++-------- docs/hackbot/agents.md | 4 ++-- docs/hackbot/api.md | 16 +++++----------- 3 files changed, 10 insertions(+), 21 deletions(-) diff --git a/docs/hackbot/actions.md b/docs/hackbot/actions.md index a4ba3f6582..1bde5bb10e 100644 --- a/docs/hackbot/actions.md +++ b/docs/hackbot/actions.md @@ -89,14 +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.** A run created with `require_review=true` always waits for a - human to click apply. Otherwise it may apply immediately, but only when the agent - also has `auto_apply_actions=True`; agent-specific consent checks still apply. - Omitting the flag defaults to `false`. The choice is stored with the run's other - inputs and read back at completion onto the `run.completed` event; `model_to_env` - withholds it from the container env, so the agent never sees it. Only an explicit - `true` holds; anything else defers to the agent's own policy, which must still opt - in through `auto_apply_actions` before anything is applied. +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. diff --git a/docs/hackbot/agents.md b/docs/hackbot/agents.md index 0ffbe6465b..099b3ae64c 100644 --- a/docs/hackbot/agents.md +++ b/docs/hackbot/agents.md @@ -80,8 +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 makes the agent eligible for automatic application; an individual - run created in `review` mode still holds its actions for a human. + 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 diff --git a/docs/hackbot/api.md b/docs/hackbot/api.md index ffbe001511..5edb60543b 100644 --- a/docs/hackbot/api.md +++ b/docs/hackbot/api.md @@ -22,17 +22,11 @@ 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. -The body also accepts `require_review: true|false`, declared once on the shared -`AgentInputs` base that every agent's input schema inherits — so it validates with the -agent's own inputs and appears in each published `/agents` schema. When true, recorded -external actions always stay pending for a human. When false (the default), the run is -only _eligible_ for automatic application: the agent must still opt in through -`AgentSpec.auto_apply_actions`, and any agent-specific consent check still applies. The -flag is persisted with the run's other inputs (JSONB, so no migration) and read back by -`finalize_run` onto the completion event, which is how the choice survives into the later -completion request. `model_to_env` withholds it from the container env, so it never -reaches the agent or its prompt. Retriggers replay the run's inputs, so a run that asked -for review is retried under review. +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.