Skip to content

Commit 0371122

Browse files
committed
fix(pipeline): avoid duplicate handoff confirmations
1 parent 5698bdb commit 0371122

8 files changed

Lines changed: 212 additions & 22 deletions

File tree

‎src/iac_code/a2a/pipeline_executor.py‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,7 @@ def __init__(
412412
backup_service: Any | None = None,
413413
aliyun_delegated_executor_factory: Any | None = None,
414414
pipeline_name: str | None = None,
415+
permission_context_getter: Callable[[], Any] | None = None,
415416
) -> None:
416417
self._task_store = task_store
417418
self._model = model
@@ -438,6 +439,7 @@ def __init__(
438439
self._backup_service = backup_service or SessionBackupService()
439440
self._aliyun_delegated_executor_factory = aliyun_delegated_executor_factory
440441
self._pipeline_name_override = pipeline_name or None
442+
self._permission_context_getter = permission_context_getter
441443

442444
def _resolve_pipeline_name(self) -> str:
443445
"""Pipeline this executor must run.
@@ -1678,7 +1680,9 @@ def _create_pipeline(
16781680
services = getattr(runtime, "aliyun_services", None)
16791681
delegated_factory = getattr(services, "delegated_executor_factory", None)
16801682
agent_loop = getattr(runtime, "agent_loop", None)
1681-
permission_context_getter = getattr(agent_loop, "_permission_context_getter", None)
1683+
permission_context_getter = self._permission_context_getter
1684+
if not callable(permission_context_getter):
1685+
permission_context_getter = getattr(agent_loop, "_permission_context_getter", None)
16821686
if not callable(permission_context_getter) and agent_loop is not None:
16831687

16841688
def permission_context_getter() -> Any:

‎src/iac_code/pipeline/engine/handoff.py‎

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ def build_handoff_summary(
2424
outcome: TerminalOutcome,
2525
context_snapshot: dict,
2626
include_fields: list[str],
27+
release_tools_any_allowed: bool | None = None,
2728
) -> str:
2829
"""Build deterministic text for continuing in normal chat after a pipeline."""
2930
included = {
@@ -43,18 +44,28 @@ def build_handoff_summary(
4344
if missing:
4445
lines.extend(["", "Missing context fields:"])
4546
lines.extend(f"- {field_name}" for field_name in missing)
46-
lines.extend(
47-
[
48-
"",
49-
"Safety requirements for normal chat:",
50-
(
51-
"- Before performing any operation that releases, deletes, or otherwise destroys a resource, "
52-
"obtain a fresh, explicit confirmation from the user in normal chat. Any confirmation given "
53-
"during the pipeline does not count."
54-
),
55-
("- Exception: pipeline-managed automatic cleanup may proceed without this additional confirmation."),
56-
"",
57-
"Use this context when answering follow-up questions after the pipeline handoff.",
58-
]
59-
)
47+
lines.extend(["", "Safety requirements for normal chat:"])
48+
if release_tools_any_allowed is False:
49+
lines.extend(
50+
[
51+
(
52+
"- For operations that release, delete, or otherwise destroy a resource, rely on the tool "
53+
"permission confirmation as the sole confirmation. Do not ask for a separate confirmation "
54+
"in normal chat, and do not proceed unless the permission request is approved."
55+
),
56+
("- Exception: pipeline-managed automatic cleanup may proceed without an additional confirmation."),
57+
]
58+
)
59+
else:
60+
lines.extend(
61+
[
62+
(
63+
"- Before performing any operation that releases, deletes, or otherwise destroys a resource, "
64+
"obtain a fresh, explicit confirmation from the user in normal chat. Any confirmation given "
65+
"during the pipeline does not count."
66+
),
67+
("- Exception: pipeline-managed automatic cleanup may proceed without this additional confirmation."),
68+
]
69+
)
70+
lines.extend(["", "Use this context when answering follow-up questions after the pipeline handoff."])
6071
return "\n".join(lines)

‎src/iac_code/pipeline/engine/pipeline_runner.py‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1093,8 +1093,50 @@ def build_normal_handoff_summary(self, completed_event_data: dict) -> str:
10931093
outcome=outcome,
10941094
context_snapshot=self.context.snapshot(),
10951095
include_fields=include_fields,
1096+
release_tools_any_allowed=self._normal_chat_release_tools_any_allowed(),
10961097
)
10971098

1099+
def _normal_chat_release_tools_any_allowed(self) -> bool | None:
1100+
permission_context = self._normal_chat_permission_context()
1101+
if permission_context is None:
1102+
return None
1103+
mode = getattr(permission_context, "mode", None)
1104+
mode_value = getattr(mode, "value", mode)
1105+
mode_name = mode_value if isinstance(mode_value, str) else None
1106+
allow_rules = getattr(permission_context, "allow_rules", {})
1107+
ask_rules = getattr(permission_context, "ask_rules", {})
1108+
deny_rules = getattr(permission_context, "deny_rules", {})
1109+
if not all(isinstance(rules, dict) for rules in (allow_rules, ask_rules, deny_rules)):
1110+
return None
1111+
1112+
for tool_name in ("aliyun_api", "ros_stack"):
1113+
if self._permission_rules_contain_tool(deny_rules, tool_name):
1114+
continue
1115+
if self._permission_rules_contain_tool(ask_rules, tool_name):
1116+
continue
1117+
if self._permission_rules_contain_tool(allow_rules, tool_name) or mode_name == "bypass_permissions":
1118+
return True
1119+
return False
1120+
1121+
@staticmethod
1122+
def _permission_rules_contain_tool(rules_by_source: dict[str, list[str]], tool_name: str) -> bool:
1123+
return any(
1124+
isinstance(rule, str) and rule.split("(", 1)[0].strip() == tool_name
1125+
for rules in rules_by_source.values()
1126+
if isinstance(rules, list)
1127+
for rule in rules
1128+
)
1129+
1130+
def _normal_chat_permission_context(self) -> Any | None:
1131+
getter = self._permission_context_getter
1132+
if not callable(getter):
1133+
return None
1134+
try:
1135+
return getter()
1136+
except Exception:
1137+
logger.debug("Failed to read permission context for normal chat handoff", exc_info=True)
1138+
return None
1139+
10981140
def mark_normal_handoff(self, status: str, failed_reason: str | None = None) -> None:
10991141
"""Record terminal pipeline-to-normal handoff metadata without deleting the sidecar."""
11001142
if not self.session:

‎src/iac_code/web/app.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -298,8 +298,8 @@ async def state_transaction(function: Callable[..., Any], /, *args: Any, **kwarg
298298
)
299299
)
300300
make_shell_runner: WebShellRunnerFactory = shell_runner_factory or (lambda: WebShellEscapeRunner(manager))
301-
make_pipeline_action_runner: WebPipelineActionRunnerFactory = (
302-
pipeline_action_runner_factory or create_pipeline_action_runner
301+
make_pipeline_action_runner: WebPipelineActionRunnerFactory = pipeline_action_runner_factory or (
302+
lambda: create_pipeline_action_runner(manager.ensure_permission_context)
303303
)
304304
command_dispatcher = WebCommandDispatcher(manager)
305305
shell_runner = make_shell_runner()

‎src/iac_code/web/pipeline_actions.py‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,11 @@ class PipelineActionResult:
9696
class A2APipelineActionRunner:
9797
"""Route Web pipeline actions through the existing A2A pipeline executor."""
9898

99-
def __init__(self, runtime_owner: Any | None = None) -> None:
99+
def __init__(
100+
self,
101+
runtime_owner: Any | None = None,
102+
permission_context_getter: Callable[[Any], Any] | None = None,
103+
) -> None:
100104
from iac_code.a2a.exposure import A2AExposureType
101105
from iac_code.a2a.metrics import NoOpA2AMetrics
102106
from iac_code.a2a.persistence import A2APersistenceStore
@@ -126,6 +130,7 @@ def __init__(self, runtime_owner: Any | None = None) -> None:
126130
self._owner = owner
127131
self._task_store = owner.task_store
128132
self._uses_web_global_defaults = uses_web_global_defaults
133+
self._permission_context_getter = permission_context_getter
129134

130135
async def startup(self) -> None:
131136
"""Start background maintenance for the fallback store owned by Web."""
@@ -361,6 +366,11 @@ def _executor_for_session(
361366
# without this the executor would always fall back to the process-wide
362367
# IAC_CODE_PIPELINE_NAME default and silently run `selling`.
363368
pipeline_name=_session_pipeline_name(session),
369+
permission_context_getter=(
370+
(lambda: self._permission_context_getter(session))
371+
if callable(getattr(self, "_permission_context_getter", None))
372+
else None
373+
),
364374
)
365375

366376
async def rebuild_permission_audit_event(
@@ -565,8 +575,10 @@ class _WebA2AContext:
565575
metadata: dict[str, Any] = {}
566576

567577

568-
def create_pipeline_action_runner() -> PipelineActionRunner:
569-
return A2APipelineActionRunner()
578+
def create_pipeline_action_runner(
579+
permission_context_getter: Callable[[Any], Any] | None = None,
580+
) -> PipelineActionRunner:
581+
return A2APipelineActionRunner(permission_context_getter=permission_context_getter)
570582

571583

572584
async def load_pipeline_snapshot(*, context_id: str | None, task_id: str | None) -> dict[str, Any] | None:

‎tests/a2a/test_pipeline_executor.py‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import threading
1010
from pathlib import Path
1111
from types import SimpleNamespace
12+
from typing import Any
1213
from unittest.mock import AsyncMock, MagicMock
1314

1415
import pytest
@@ -230,6 +231,38 @@ def _pipeline_executor(*, aliyun_delegated_executor_factory=None, candidate_pres
230231
)
231232

232233

234+
def test_create_pipeline_prefers_explicit_permission_context_getter(monkeypatch: pytest.MonkeyPatch) -> None:
235+
import iac_code.a2a.pipeline_executor as pipeline_executor_module
236+
237+
explicit_permission_context = object()
238+
runtime_permission_context = object()
239+
captured: dict[str, Any] = {}
240+
241+
def fake_create_pipeline(*_args: Any, **kwargs: Any) -> object:
242+
captured.update(kwargs)
243+
return object()
244+
245+
monkeypatch.setattr(pipeline_executor_module, "create_pipeline", fake_create_pipeline)
246+
executor = _pipeline_executor()
247+
executor._permission_context_getter = lambda: explicit_permission_context
248+
249+
executor._create_pipeline(
250+
session_id="session-1",
251+
cwd="/workspace",
252+
runtime=SimpleNamespace(
253+
provider_manager=object(),
254+
tool_registry=object(),
255+
agent_loop=SimpleNamespace(_permission_context_getter=lambda: runtime_permission_context),
256+
aliyun_services=None,
257+
),
258+
session_storage=MagicMock(),
259+
prerequisite_metadata={},
260+
pipeline_name="selling",
261+
)
262+
263+
assert captured["permission_context_getter"]() is explicit_permission_context
264+
265+
233266
@pytest.mark.asyncio
234267
async def test_pipeline_permission_backup_records_staged_generation_before_publication() -> None:
235268
task_store = MagicMock()

‎tests/pipeline/engine/test_pipeline_handoff.py‎

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
from pathlib import Path
22
from textwrap import dedent
3+
from types import SimpleNamespace
34
from unittest.mock import MagicMock
45

56
import yaml
@@ -8,7 +9,11 @@
89
from iac_code.pipeline.engine.pipeline_runner import PipelineRunner
910

1011

11-
def _make_runner(tmp_path: Path, on_complete: dict | None = None) -> PipelineRunner:
12+
def _make_runner(
13+
tmp_path: Path,
14+
on_complete: dict | None = None,
15+
permission_mode: str | None = None,
16+
) -> PipelineRunner:
1217
body = {
1318
"name": "test",
1419
"context_dependencies": {
@@ -31,13 +36,15 @@ def _make_runner(tmp_path: Path, on_complete: dict | None = None) -> PipelineRun
3136
(tmp_path / "prompts").mkdir()
3237
(tmp_path / "prompts" / "step.md").write_text("step", encoding="utf-8")
3338

39+
permission_context_getter = (lambda: SimpleNamespace(mode=permission_mode)) if permission_mode is not None else None
3440
return PipelineRunner(
3541
pipeline_dir=tmp_path,
3642
provider_manager=MagicMock(),
3743
base_tool_registry=MagicMock(),
3844
session_storage=MagicMock(),
3945
session_id="session",
4046
cwd=str(tmp_path),
47+
permission_context_getter=permission_context_getter,
4148
)
4249

4350

@@ -133,6 +140,33 @@ def test_build_handoff_summary_requires_new_confirmation_for_resource_release_ex
133140
assert "pipeline-managed automatic cleanup may proceed without this additional confirmation" in summary
134141

135142

143+
def test_build_handoff_summary_uses_permission_confirmation_when_release_tools_are_not_allowed():
144+
summary = build_handoff_summary(
145+
pipeline_name="selling",
146+
outcome="completed",
147+
context_snapshot={},
148+
include_fields=[],
149+
release_tools_any_allowed=False,
150+
)
151+
152+
assert "tool permission confirmation as the sole confirmation" in summary
153+
assert "Do not ask for a separate confirmation in normal chat" in summary
154+
assert "obtain a fresh, explicit confirmation" not in summary
155+
156+
157+
def test_build_handoff_summary_keeps_chat_confirmation_when_a_release_tool_is_allowed():
158+
summary = build_handoff_summary(
159+
pipeline_name="selling",
160+
outcome="completed",
161+
context_snapshot={},
162+
include_fields=[],
163+
release_tools_any_allowed=True,
164+
)
165+
166+
assert "obtain a fresh, explicit confirmation" in summary
167+
assert "tool permission confirmation as the sole confirmation" not in summary
168+
169+
136170
def test_runner_should_switch_to_normal_for_completed_policy(tmp_path):
137171
runner = _make_runner(tmp_path, _switch_policy("completed"))
138172

@@ -181,3 +215,36 @@ def test_runner_build_normal_handoff_summary_uses_configured_context_values(tmp_
181215
assert "Outcome: completed" in summary
182216
assert '"summary": "deploy nginx"' in summary
183217
assert "Missing context fields:\n- architecture" in summary
218+
219+
220+
def test_runner_build_normal_handoff_summary_uses_current_permission_rules(tmp_path):
221+
runner = _make_runner(tmp_path, _switch_policy("completed"), permission_mode="default")
222+
runner._permission_context_getter = lambda: SimpleNamespace(
223+
mode="default",
224+
allow_rules={
225+
"user_settings": [
226+
"bash(**)",
227+
"read_file",
228+
]
229+
},
230+
ask_rules={"user_settings": ["ros_stack", "aliyun_api"]},
231+
deny_rules={},
232+
)
233+
234+
summary = runner.build_normal_handoff_summary({"total_steps": 1})
235+
236+
assert "tool permission confirmation as the sole confirmation" in summary
237+
238+
239+
def test_runner_build_normal_handoff_summary_uses_chat_confirmation_when_a_release_tool_is_allowed(tmp_path):
240+
runner = _make_runner(tmp_path, _switch_policy("completed"), permission_mode="default")
241+
runner._permission_context_getter = lambda: SimpleNamespace(
242+
mode="default",
243+
allow_rules={"user_settings": ["aliyun_api"]},
244+
ask_rules={"user_settings": ["ros_stack"]},
245+
deny_rules={},
246+
)
247+
248+
summary = runner.build_normal_handoff_summary({"total_steps": 1})
249+
250+
assert "obtain a fresh, explicit confirmation" in summary

‎tests/web/test_pipeline_actions.py‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,11 @@ async def get_task_record(self, _task_id: str) -> Any:
108108
return SimpleNamespace(state="working")
109109

110110

111-
async def _executor_kwargs_for_session(monkeypatch: pytest.MonkeyPatch, session: Any) -> dict[str, Any]:
111+
async def _executor_kwargs_for_session(
112+
monkeypatch: pytest.MonkeyPatch,
113+
session: Any,
114+
permission_context_getter: Any = None,
115+
) -> dict[str, Any]:
112116
import iac_code.a2a.pipeline_executor as pipeline_executor_module
113117
from iac_code.web import pipeline_actions
114118

@@ -122,6 +126,7 @@ async def no_snapshot(**_kwargs: Any) -> None:
122126
runner = A2APipelineActionRunner.__new__(A2APipelineActionRunner)
123127
runner._task_store = _StubTaskStore()
124128
runner._uses_web_global_defaults = False
129+
runner._permission_context_getter = permission_context_getter
125130
runner._owner = SimpleNamespace(
126131
model="qwen3.6-plus",
127132
metrics=None,
@@ -175,6 +180,22 @@ async def test_execute_leaves_the_process_default_when_the_session_has_no_select
175180
assert kwargs["pipeline_name"] is None
176181

177182

183+
@pytest.mark.asyncio
184+
async def test_execute_passes_the_web_session_permission_context_to_pipeline(
185+
monkeypatch: pytest.MonkeyPatch,
186+
) -> None:
187+
permission_context = object()
188+
session = _pipeline_session(permission_context=permission_context)
189+
190+
kwargs = await _executor_kwargs_for_session(
191+
monkeypatch,
192+
session,
193+
permission_context_getter=lambda current_session: current_session.permission_context,
194+
)
195+
196+
assert kwargs["permission_context_getter"]() is permission_context
197+
198+
178199
@pytest.mark.asyncio
179200
@pytest.mark.parametrize("stored_name", ["no_such_pipeline", " "])
180201
async def test_execute_falls_back_when_the_session_pipeline_is_unknown(

0 commit comments

Comments
 (0)