fix: add security_risk and summary to tool examples for non-native function calling

[ixchio]

Summary

This PR fixes issue #2175 where models using prompt-based function calling (native_tool_calling=False) would fail to provide the security_risk parameter because the examples in the system prompt didn't demonstrate it.

Problem

When using weaker models (like qwen2.5-coder:7b) that don't support native function calling, the SDK falls back to prompt-based function calling. This injects tool definitions and examples directly into the system prompt. However, these examples were missing the security_risk and summary parameters, causing validation errors:

Error validating args {"command": "pwd && ls"} for tool 'terminal': 
Failed to provide security_risk field in tool 'terminal'

The model didn't know to include these fields because the examples didn't show them.

Solution

Added security_risk and summary parameters to all tool examples in:

• openhands-sdk/openhands/sdk/llm/mixins/fn_call_converter.py (TOOL_EXAMPLES dict)
• openhands-sdk/openhands/sdk/agent/prompts/in_context_learning_example.j2 (template)

Now the examples teach models to include these parameters:

<function=terminal>
<parameter=command>pwd && ls</parameter>
<parameter=security_risk>LOW</parameter>
<parameter=summary>Check current directory and list files</parameter>
</function>

Details

• security_risk is required when LLMSecurityAnalyzer is configured
• summary is optional but recommended for action transparency
• Both parameters are always added to tool schemas via add_security_risk_prediction=True

Testing

• All existing tests pass (27 fn_call_converter tests, 21 security_risk extraction tests)
• Pre-commit hooks pass

Fixes #2175

[ixchio]

Note on Failing API Breakage Checks

The failing API breakage checks are not related to this PR's changes.

The failure is caused by PR #2243 (fix: override server_image default to None in DockerDevWorkspace) which was intentionally merged to main on Feb 28. This PR changed DockerDevWorkspace.server_image default from a specific image to None, which Griffe detects as a breaking change.

My changes only modify:

• String constants in TOOL_EXAMPLES dictionary (internal prompt examples)
• The in_context_learning_example.j2 template (prompt content)

Neither of these affect the public SDK API surface.

For openhands-sdk: "No breaking changes detected" ✓

[ixchio]

@all-hands-bot roast it

[enyst]

Hey, thank you for the contribution! Yes… yes I think missing security risk from prompts was causing models to miss it in responses.

I’m curious though, is security risk always supposed to be present? It’s worth making sure what happens when the security analyzer is disabled.

On another note, about the summary, I wonder if including it affects performance, it might be worth to run a mini-evaluation. Maybe it’s too small and obvious 🤷‍♂️ but I don’t know

[enyst]

@OpenHands Do a /codereview-roasted on this PR, after you read all comments, and try to understand deeply how the code works. Post your review directly on github as a comment.

[openhands-ai]

I'm on it! enyst can track my progress at all-hands.dev

[enyst]

Taste rating: 🟡 Acceptable (but sloppy around the exact bug you’re claiming to fix)

You’re fixing a real problem (prompt-mocked tool calling models missing security_risk), and the general direction is fine. But the change is incomplete in a way that will keep the same class of failure alive for at least one of the default tools.

[CRITICAL ISSUES]

• [openhands-sdk/openhands/sdk/llm/mixins/fn_call_converter.py, Lines 351–380] Missing security_risk in task_tracker examples

  • TaskTrackerTool is explicitly readOnlyHint=False (see openhands-tools/openhands/tools/task_tracker/definition.py), and make_llm_completion() always exposes security_risk in tool schemas.
  • When LLMSecurityAnalyzer is enabled, Agent._extract_security_risk() raises if the field is missing.
  • Your examples for task_tracker still don’t show security_risk, so prompt-based FC models will keep “learning” to omit it, and you’ll get the same validation error — just for a different tool.
  • Actionable fix: add <parameter=security_risk>…</parameter> to both task_tracker examples (view and plan).

• [openhands-sdk/openhands/sdk/agent/prompts/in_context_learning_example.j2, Lines 166–168] finish example is invalid (missing required message)

  • FinishAction requires message: str (openhands-sdk/openhands/sdk/tool/builtins/finish.py).
  • This template shows <function=finish> without a message parameter, which teaches models the wrong call shape.
  • Even if this file isn’t currently wired into the non-native FC path, it’s shipped in the SDK and is now being edited, so it should not contain known-invalid examples.
  • Actionable fix: include <parameter=message>…</parameter> in that example (and keep summary if you want).

[IMPROVEMENT OPPORTUNITIES]

• [openhands-sdk/openhands/sdk/llm/mixins/fn_call_converter.py, Lines 88–90] Misleading comment about “always added” fields

  • summary is always added to the schema.
  • security_risk is conditionally added (suppressed for read-only tools via ToolDefinition._get_tool_schema).
  • Not a huge deal, but comments that state “always” while the code does “sometimes” rot fast.

• Token/perf tradeoff for summary

  • You’re now explicitly demonstrating summary everywhere, which will nudge weaker models to always emit it. That’s extra tokens every tool call.
  • It’s not a correctness problem (agent will auto-generate if missing), but it is a throughput/cost knob. Worth a quick sanity check on any evals that are sensitive to token budget.

[TESTING GAPS]

• No test proves the thing you’re fixing for all default tools.
  • If this PR is meant to prevent “missing security_risk” regressions, add a focused test that exercises the non-native FC path with task_tracker present and asserts the model output is accepted when LLMSecurityAnalyzer is enabled.

Verdict: ✅ likely worth merging after the missing task_tracker.security_risk and the invalid finish example are fixed.

Key insight: If you’re going to “teach the model” via examples, leaving one default tool with the old broken example defeats the entire point.

[ixchio]

Addressed the review feedback:

Fixes

1. Added security_risk to task_tracker examples (fn_call_converter.py)

   • task_tracker has readOnlyHint=False, so it requires security_risk when LLMSecurityAnalyzer is enabled
   • Added <parameter=security_risk>LOW</parameter> to both view and plan examples

2. Added required message parameter to finish example (in_context_learning_example.j2)

   • FinishAction.message is a required field
   • The example was teaching models the wrong call shape

Note on security_risk for read-only tools

To clarify @enyst's question: security_risk is NOT always required. It's only added to tool schemas when:

• add_security_risk_prediction=True (default)
• AND the tool is NOT read-only (readOnlyHint != True)

finish has readOnlyHint=True, so it doesn't need security_risk in examples. The same applies to other read-only tools like browser_get_state, browser_get_content, etc.

Regarding @enyst's question about summary affecting performance - it's a reasonable concern. However, summary is optional and auto-generated if missing, so models can skip it when they want. The examples just demonstrate the full format.

[enyst]

Please don't worry about updating, nor about the checks (fixing those separately, they're only unable to comment at the moment)

[xingyaoww]

Thank you!