Merge branch 'main' into feat/pr-review-sub-agent-delegation

Author: xingyaoww

.github/scripts/check_sdk_api_breakage.py

@@ -33,7 +33,6 @@ class member must have been marked deprecated in the *previous* release using
 import ast
 import json
 import os
-import re
 import subprocess
 import sys
 import tomllib
@@ -376,71 +375,88 @@ def ensure_griffe() -> None:
         raise SystemExit(1)
 
 
-def _strip_balanced_param(text: str, param: str) -> str:
-    """Remove a keyword parameter whose value may contain nested delimiters.
+FIELD_METADATA_KWARGS = frozenset(
+    {
+        "deprecated",
+        "description",
+        "examples",
+        "json_schema_extra",
+        "title",
+    }
+)
 
-    Handles values like ``json_schema_extra={'key': {'nested': True}}`` where a
-    simple regex cannot reliably match the balanced braces/parens/brackets.
 
-    Returns *text* with the ``param=<value>`` fragment (and any surrounding
-    comma) removed.
-    """
-    pattern = re.compile(rf",?\s*{re.escape(param)}\s*=\s*")
-    match = pattern.search(text)
-    if not match:
-        return text
-
-    start = match.start()
-    pos = match.end()
-    if pos >= len(text):
-        return text
-
-    # Track balanced delimiters to find where the value ends.
-    openers = {"(": ")", "[": "]", "{": "}"}
-    closers = {")", "]", "}"}
-    stack: list[str] = []
+def _escape_newlines_in_string_literals(text: str) -> str:
+    """Escape literal newlines that appear inside quoted string literals."""
+    chars: list[str] = []
     in_string: str | None = None
+    escaped = False
+
+    for ch in text:
+        if in_string is None:
+            chars.append(ch)
+            if ch in {"'", '"'}:
+                in_string = ch
+            continue
+
+        if escaped:
+            chars.append(ch)
+            escaped = False
+            continue
 
-    while pos < len(text):
-        ch = text[pos]
-
-        # Handle string literals (skip their contents).
-        if in_string:
-            if ch == "\\" and pos + 1 < len(text):
-                pos += 2
-                continue
-            if ch == in_string:
-                in_string = None
-            pos += 1
+        if ch == "\\":
+            chars.append(ch)
+            escaped = True
             continue
 
-        if ch in ("'", '"'):
-            in_string = ch
-            pos += 1
+        if ch == in_string:
+            chars.append(ch)
+            in_string = None
             continue
 
-        if ch in openers:
-            stack.append(openers[ch])
-            pos += 1
+        if ch == "\n":
+            chars.append("\\n")
             continue
 
-        if ch in closers:
-            if stack:
-                stack.pop()
-                pos += 1
-                if not stack:
-                    break
-                continue
-            # Unmatched closer — end of value.
-            break
+        chars.append(ch)
+
+    return "".join(chars)
+
+
+def _parse_field_call(value: object) -> ast.Call | None:
+    """Parse a stringified Pydantic ``Field(...)`` value into an AST call."""
+    try:
+        expr = ast.parse(
+            _escape_newlines_in_string_literals(str(value)),
+            mode="eval",
+        ).body
+    except SyntaxError:
+        return None
+
+    if not isinstance(expr, ast.Call):
+        return None
+
+    func = expr.func
+    if isinstance(func, ast.Name):
+        func_name = func.id
+    elif isinstance(func, ast.Attribute):
+        func_name = func.attr
+    else:
+        return None
 
-        # At depth 0, a comma or closing paren ends the value.
-        if not stack and ch in (",", ")"):
-            break
+    if func_name != "Field":
+        return None
 
-        pos += 1
+    return expr
 
-    return text[:start] + text[pos:]
+
+def _filter_field_metadata_kwargs(call: ast.Call) -> ast.Call:
+    """Return a copy of a ``Field(...)`` call without metadata-only kwargs."""
+    return ast.Call(
+        func=call.func,
+        args=call.args,
+        keywords=[kw for kw in call.keywords if kw.arg not in FIELD_METADATA_KWARGS],
+    )
 
 
 def _is_field_metadata_only_change(old_val: object, new_val: object) -> bool:
@@ -453,43 +469,18 @@ def _is_field_metadata_only_change(old_val: object, new_val: object) -> bool:
     Returns:
         True if both values are Field() calls and only metadata parameters differ.
     """
-    old_str = str(old_val)
-    new_str = str(new_val)
-
-    if not (old_str.startswith("Field(") and new_str.startswith("Field(")):
+    old_call = _parse_field_call(old_val)
+    new_call = _parse_field_call(new_val)
+    if old_call is None or new_call is None:
         return False
 
-    # Simple metadata parameters whose values are always plain quoted strings
-    # or simple literals.
-    # See https://docs.pydantic.dev/latest/api/fields/#pydantic.fields.Field
-    simple_metadata_patterns = {
-        "description": r'([\'"])([^\'"]*?)\1',
-        "title": r'([\'"])([^\'"]*?)\1',
-        "examples": r'([\'"])([^\'"]*?)\1',
-        "deprecated": r"(?:True|False|None|'[^']*'|\"[^\"]*\")",
-    }
-
-    # Parameters whose values can be complex nested structures (dicts, function
-    # calls, etc.) and need balanced-delimiter parsing instead of a regex.
-    balanced_params = ("json_schema_extra",)
-
-    def _normalize(value: str) -> str:
-        normalized = value
-
-        for param, value_pattern in simple_metadata_patterns.items():
-            pattern = rf",?\s*{param}\s*=\s*{value_pattern}"
-            normalized = re.sub(pattern, "", normalized)
-
-        for param in balanced_params:
-            normalized = _strip_balanced_param(normalized, param)
-
-        normalized = re.sub(r"\(\s*,", "(", normalized)
-        normalized = re.sub(r",\s*\)", ")", normalized)
-        normalized = re.sub(r",\s*,", ", ", normalized)
-        normalized = re.sub(r"\s+", " ", normalized)
-        return normalized.strip()
-
-    return _normalize(old_str) == _normalize(new_str)
+    return ast.dump(
+        _filter_field_metadata_kwargs(old_call),
+        include_attributes=False,
+    ) == ast.dump(
+        _filter_field_metadata_kwargs(new_call),
+        include_attributes=False,
+    )
 
 
 def _member_deprecation_metadata(

.github/workflows/pr-review-evaluation.yml

@@ -28,7 +28,7 @@ jobs:
         steps:
             - name: Download review trace artifact
               id: download-trace
-              uses: dawidd6/action-download-artifact@v19
+              uses: dawidd6/action-download-artifact@v20
               continue-on-error: true
               with:
                   workflow: pr-review-by-openhands.yml

.github/workflows/qa-changes-evaluation.yml

@@ -28,7 +28,7 @@ jobs:
         steps:
             - name: Download QA trace artifact
               id: download-trace
-              uses: dawidd6/action-download-artifact@v19
+              uses: dawidd6/action-download-artifact@v20
               continue-on-error: true
               with:
                   workflow: qa-changes-by-openhands.yml

AGENTS.md

@@ -140,6 +140,10 @@ consult each relevant package-level AGENTS.md.
   Pydantic `Field(...)` declarations as non-breaking, including adding,
   removing, or editing `description`, `title`, `examples`,
   `json_schema_extra`, and `deprecated` kwargs.
+- The SDK API breakage checker compares stringified `Field(...)` values by
+  parsing them as Python expressions after escaping literal newlines inside
+  quoted strings; this avoids false positives on multiline descriptions that
+  include embedded quotes like `'security_policy.j2'`.
 - For public REST APIs, read
   [openhands-agent-server/AGENTS.md](openhands-agent-server/AGENTS.md).
   REST contract breaks need a deprecation notice and a runway of

openhands-sdk/openhands/sdk/agent/base.py

@@ -158,14 +158,14 @@ class AgentBase(DiscriminatedUnionMixin, ABC):
             "- An absolute path (e.g., '/path/to/custom_prompt.j2')"
         ),
     )
-    security_policy_filename: str | None = Field(
+    security_policy_filename: str = Field(
         default="security_policy.j2",
         description=(
-            "Security policy template filename. Can be:\n"
+            "Security policy template filename. Can be either:\n"
             "- A relative filename (e.g., 'security_policy.j2') loaded from the "
             "agent's prompts directory\n"
             "- An absolute path (e.g., '/path/to/custom_security_policy.j2')\n"
-            "- Empty string or None to disable security policy"
+            "- Empty string to disable security policy"
         ),
     )
     system_prompt_kwargs: dict[str, object] = Field(
@@ -179,6 +179,11 @@ class AgentBase(DiscriminatedUnionMixin, ABC):
     def _validate_system_prompt_fields(cls, data: Any) -> Any:
         if not isinstance(data, dict):
             return data
+        if (
+            "security_policy_filename" in data
+            and data["security_policy_filename"] is None
+        ):
+            data["security_policy_filename"] = ""
         has_inline = data.get("system_prompt") is not None
         has_custom_filename = (
             "system_prompt_filename" in data

tests/cross/test_check_sdk_api_breakage.py

@@ -555,6 +555,27 @@ def test_is_field_metadata_only_change_long_description():
     assert _is_field_metadata_only_change(old, new) is True
 
 
+def test_is_field_metadata_only_change_multiline_description_with_quotes():
+    """Multiline descriptions with embedded quotes are metadata-only changes."""
+    old = (
+        "Field(default='security_policy.j2', description=\"Security policy "
+        "template filename. Can be either:\n"
+        "- A relative filename (e.g., 'security_policy.j2') loaded from the "
+        "agent's prompts directory\n"
+        "- An absolute path (e.g., '/path/to/custom_security_policy.j2')\")"
+    )
+    new = (
+        "Field(default='security_policy.j2', description=\"Security policy "
+        "template filename. Can be either:\n"
+        "- A relative filename (e.g., 'security_policy.j2') loaded from the "
+        "agent's prompts directory\n"
+        "- An absolute path (e.g., '/path/to/custom_security_policy.j2')\n"
+        '- Empty string to disable security policy")'
+    )
+
+    assert _is_field_metadata_only_change(old, new) is True
+
+
 def test_is_field_metadata_only_change_deprecated_bool_only():
     """Changing only Field deprecated metadata is detected as metadata-only."""
     old = "Field(default=False, deprecated=False)"
@@ -699,7 +720,68 @@ def test_field_description_change_is_not_breaking(tmp_path):
         new_root,
         _SDK_CFG,
     )
-    # Field description changes should NOT count as breaking
+    assert total_breaks == 0
+    assert undeprecated == 0
+
+
+def test_field_multiline_description_with_quotes_is_not_breaking(tmp_path):
+    """Multiline descriptions with embedded quotes should not be breaking."""
+    old_pkg = _write_pkg_init(tmp_path, "old", ["Config"])
+    new_pkg = _write_pkg_init(tmp_path, "new", ["Config"])
+
+    old_init = old_pkg / "__init__.py"
+    new_init = new_pkg / "__init__.py"
+
+    old_init.write_text(
+        old_init.read_text()
+        + "\nfrom pydantic import BaseModel, Field\n\n"
+        + "class Config(BaseModel):\n"
+        + "    policy: str = Field(\n"
+        + "        default='security_policy.j2',\n"
+        + "        description=(\n"
+        + '            "Security policy template filename. Can be either:\\n"\n'
+        + (
+            '            "- A relative filename (e.g., '
+            "'security_policy.j2') loaded from \"\n"
+        )
+        + '            "the agent\'s prompts directory\\n"\n'
+        + (
+            '            "- An absolute path (e.g., '
+            "'/path/to/custom_security_policy.j2')\"\n"
+        )
+        + "        ),\n"
+        + "    )\n"
+    )
+    new_init.write_text(
+        new_init.read_text()
+        + "\nfrom pydantic import BaseModel, Field\n\n"
+        + "class Config(BaseModel):\n"
+        + "    policy: str = Field(\n"
+        + "        default='security_policy.j2',\n"
+        + "        description=(\n"
+        + '            "Security policy template filename. Can be either:\\n"\n'
+        + (
+            '            "- A relative filename (e.g., '
+            "'security_policy.j2') loaded from \"\n"
+        )
+        + '            "the agent\'s prompts directory\\n"\n'
+        + (
+            '            "- An absolute path (e.g., '
+            "'/path/to/custom_security_policy.j2')\\n\"\n"
+        )
+        + '            "- Empty string to disable security policy"\n'
+        + "        ),\n"
+        + "    )\n"
+    )
+
+    old_root = griffe.load("openhands.sdk", search_paths=[str(tmp_path / "old")])
+    new_root = griffe.load("openhands.sdk", search_paths=[str(tmp_path / "new")])
+
+    total_breaks, undeprecated = _prod._compute_breakages(
+        old_root,
+        new_root,
+        _SDK_CFG,
+    )
     assert total_breaks == 0
     assert undeprecated == 0
 

tests/sdk/agent/test_security_policy_integration.py

@@ -65,6 +65,25 @@ def test_security_policy_in_system_message():
     assert "AI assistant (OpenHands)" not in system_message
 
 
+def test_none_security_policy_filename_disables_policy_without_null_public_value():
+    """Test that None input disables the policy without exposing a null contract."""
+    agent = Agent.model_validate(
+        {
+            "llm": LLM(
+                usage_id="test-llm",
+                model="test-model",
+                api_key=SecretStr("test-key"),
+                base_url="http://test",
+            ),
+            "security_policy_filename": None,
+        }
+    )
+
+    assert agent.security_policy_filename == ""
+    assert agent.model_dump()["security_policy_filename"] == ""
+    assert "🔐 Security Policy" not in agent.static_system_message
+
+
 def test_custom_security_policy_in_system_message():
     """Test that custom security policy filename is used in system message."""
     # Create a temporary directory for test files