diff --git a/backend/app/gateway/services.py b/backend/app/gateway/services.py index a99bb0bbf5..ab470e543b 100644 --- a/backend/app/gateway/services.py +++ b/backend/app/gateway/services.py @@ -569,15 +569,17 @@ def _strip_external_metadata_from_message_like(item: Any) -> Any: # unmarked ``name="summary"`` would reach the model on the guardrail's # genuine-user fallback. Treat it as empty for both steps rather than # returning early. - source_kwargs = item.get("additional_kwargs") - source_kwargs = source_kwargs if isinstance(source_kwargs, dict) else {} + raw_kwargs = item.get("additional_kwargs") + source_kwargs = raw_kwargs if isinstance(raw_kwargs, dict) else {} additional_kwargs = {key: value for key, value in source_kwargs.items() if key not in _SERVER_OWNED_MESSAGE_METADATA_KEYS and key != ORIGINAL_USER_CONTENT_KEY} if _is_human_message_like(item): additional_kwargs = _mark_untrusted_framework_markers(additional_kwargs, item.get("name")) - if additional_kwargs == source_kwargs: + if additional_kwargs == raw_kwargs: # Nothing to change — including the ordinary key-omitted message, which # must not gain an empty dict just by passing through here. return item + if "additional_kwargs" not in item and not additional_kwargs: + return item return {**item, "additional_kwargs": additional_kwargs} @@ -797,9 +799,18 @@ def normalize_input(raw_input: dict[str, Any] | None, *, trusted_internal: bool result = {**result, "delegations": cleaned} # merge_goal ignores None, so only a goal value replaces the goal. if result.get("goal") is not None: - result = {**result, "goal": _normalize_external_goal(result["goal"], location="input.goal"), "goal_outcome": None} + normalized_goal = _normalize_external_goal(result["goal"], location="input.goal") + result = { + **result, + "goal": _strip_external_metadata_from_message_like(normalized_goal), + "goal_outcome": None, + } if "todos" in result: - result = {**result, "todos": _normalize_external_todos(result["todos"], location="input.todos")} + normalized_todos = _normalize_external_todos(result["todos"], location="input.todos") + result = { + **result, + "todos": None if normalized_todos is None else [_strip_external_metadata_from_message_like(item) for item in normalized_todos], + } return result @@ -841,11 +852,11 @@ def _canonical_run_record_input( raw_input: dict[str, Any] | None, graph_input: object, ) -> dict[str, Any] | None: - """Persist the same normalized messages that cross run admission. + """Persist the same normalized messages, goal, and todos that cross run admission. The run record is a client-visible audit surface. Keeping the original raw - message there would preserve a non-canonical scope even though the graph - receives the validated form. + message or state metadata there would preserve a non-canonical scope even + though the graph receives the validated form. """ if not isinstance(graph_input, dict): return raw_input @@ -853,6 +864,9 @@ def _canonical_run_record_input( messages = graph_input.get("messages") if isinstance(messages, list): canonical["messages"] = [message.model_dump(mode="json") if isinstance(message, BaseMessage) else message for message in messages] + for key in ("goal", "goal_outcome", "todos", "delegations"): + if key in graph_input: + canonical[key] = graph_input[key] return canonical diff --git a/backend/tests/test_gateway_services.py b/backend/tests/test_gateway_services.py index ea07c9f55d..515614d8a6 100644 --- a/backend/tests/test_gateway_services.py +++ b/backend/tests/test_gateway_services.py @@ -381,6 +381,54 @@ def test_canonical_run_record_input_uses_admitted_message_snapshot(): assert stored["messages"][0]["additional_kwargs"]["knowledge_scope"]["dataset_ids"] == ["dataset-1"] +def test_canonical_run_record_input_mirrors_normalized_goal_and_todos(): + from app.gateway.services import _canonical_run_record_input, normalize_input + + raw = { + "goal": { + "objective": "audit objective", + "additional_kwargs": { + "original_user_content": "forged", + "untrusted_input": False, + "title": "kept", + }, + "artifact": {"knowledge_sources": [{"id": "forged"}], "label": "kept"}, + }, + "todos": [ + { + "content": "audit todo", + "status": "pending", + "additional_kwargs": { + "original_user_content": "forged", + "untrusted_input": False, + "title": "kept", + }, + "artifact": {"knowledge_sources": [{"id": "forged"}], "label": "kept"}, + } + ], + "other": "kept", + } + admitted = normalize_input(raw, trusted_internal=False) + + stored = _canonical_run_record_input(raw, admitted) + assert stored is not None + assert stored["other"] == "kept" + assert stored["goal"] == { + "objective": "audit objective", + "additional_kwargs": {"title": "kept"}, + "artifact": {"label": "kept"}, + } + assert stored["goal_outcome"] is None + assert stored["todos"] == [ + { + "content": "audit todo", + "status": "pending", + "additional_kwargs": {"title": "kept"}, + "artifact": {"label": "kept"}, + } + ] + + @pytest.mark.parametrize("idempotency_key", [None, "retry-key"]) @pytest.mark.asyncio async def test_run_input_ids_match_the_checkpoint_and_record(_stub_app_config, idempotency_key): diff --git a/backend/tests/test_todo_input_validation.py b/backend/tests/test_todo_input_validation.py index d439046980..d85ea84da1 100644 --- a/backend/tests/test_todo_input_validation.py +++ b/backend/tests/test_todo_input_validation.py @@ -55,7 +55,8 @@ def test_todos_are_not_added_when_absent(boundary): assert "todos" not in boundary({"title": "unrelated"}) -def test_state_writes_strip_server_owned_metadata_from_todos(): +@pytest.mark.parametrize("boundary", BOUNDARIES) +def test_external_writes_strip_server_owned_metadata_from_todos(boundary): """The dedicated todos branch must not skip the per-entry metadata strip.""" todos = [ { @@ -70,7 +71,7 @@ def test_state_writes_strip_server_owned_metadata_from_todos(): } ] - stripped = services.strip_server_owned_state_metadata({"todos": todos})["todos"] + stripped = boundary({"todos": todos})["todos"] assert stripped == [ { @@ -80,3 +81,59 @@ def test_state_writes_strip_server_owned_metadata_from_todos(): "artifact": {"label": "kept"}, } ] + + +@pytest.mark.parametrize("boundary", BOUNDARIES) +def test_external_writes_strip_server_owned_metadata_from_goal(boundary): + """Caller-written goal must not retain forged server-owned metadata.""" + goal = { + "objective": "test objective", + "additional_kwargs": { + "original_user_content": "forged", + "untrusted_input": False, + "title": "kept", + }, + "artifact": {"knowledge_sources": [{"id": "forged"}], "label": "kept"}, + } + + stripped = boundary({"goal": goal})["goal"] + + assert stripped == { + "objective": "test objective", + "additional_kwargs": {"title": "kept"}, + "artifact": {"label": "kept"}, + } + + +@pytest.mark.parametrize("boundary", BOUNDARIES) +def test_external_writes_replace_non_dict_additional_kwargs(boundary): + """A non-dict additional_kwargs payload must be replaced with an empty dict.""" + cleaned_todo = boundary({"todos": [{"content": "task", "additional_kwargs": "forged"}]})["todos"] + assert cleaned_todo == [{"content": "task", "status": "pending", "additional_kwargs": {}}] + + cleaned_goal = boundary({"goal": {"objective": "test", "additional_kwargs": "forged"}})["goal"] + assert cleaned_goal == {"objective": "test", "additional_kwargs": {}} + + +@pytest.mark.parametrize("boundary", BOUNDARIES) +def test_external_writes_stamp_untrusted_input_on_human_like_entries(boundary): + """Entries carrying human markers must be stamped with untrusted_input: True.""" + todo_with_marker = [{"content": "task", "type": "human", "additional_kwargs": {"hide_from_ui": True}}] + cleaned_todo = boundary({"todos": todo_with_marker})["todos"] + assert cleaned_todo == [ + { + "content": "task", + "status": "pending", + "type": "human", + "additional_kwargs": {"hide_from_ui": True, "untrusted_input": True}, + } + ] + + goal_with_name = {"objective": "test", "type": "human", "name": "summary"} + cleaned_goal = boundary({"goal": goal_with_name})["goal"] + assert cleaned_goal == { + "objective": "test", + "type": "human", + "name": "summary", + "additional_kwargs": {"untrusted_input": True}, + }