Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 38 additions & 13 deletions src/benchflow/trajectories/metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@
)
_SKILL_RESULT_MARKER = "[skill:"

# opencode renders a skill invocation as ``kind == "other"`` / ``title ==
# "skill"`` and returns the skill body wrapped in a ``<skill_content>`` element
# naming the skill. That envelope opens the tool result, so it is anchored with
# ``\A`` for the same reason as the OpenHands header above: a tool whose output
# merely quotes the tag mid-stream is not a skill invocation.
_SKILL_CONTENT_ENVELOPE_RE = re.compile(
r"\A\s*<skill_content\s+name\s*=\s*\"[^\"]+\"", re.IGNORECASE
)

# Only these unclassified tool kinds are eligible for content sniffing. Any tool
# carrying a real ACP kind (read, edit, execute, search, fetch, ...) is trusted
# as-is and never reinterpreted from its output text.
Expand Down Expand Up @@ -79,19 +88,26 @@ def _event_tool_name(event: Mapping[str, Any]) -> str:


def content_contains_skill_invocation_tool(content: Any) -> bool:
"""Return whether tool-call content is an OpenHands invoke-skill result.

Requires the structured legacy envelope: a tool-result text block that
*begins* with the ``Tool: invoke_skill`` / ``Tool: activate_skill`` header
and carries a ``[skill: ...]`` marker. The anchored header is what
distinguishes a genuine invoke-skill tool result from ordinary output that
merely quotes such text. This is intentionally narrow; it is the only
text-derived path and is paired with the no-skill experiment-health
"""Return whether tool-call content is a skill-invocation tool result.

Two structured envelopes are recognized, both anchored to the start of a
tool-result text block:

* OpenHands legacy — a ``Tool: invoke_skill`` / ``Tool: activate_skill``
header carrying a ``[skill: ...]`` marker.
* opencode — a ``<skill_content name="...">`` element wrapping the skill
body.

The anchoring is what distinguishes a genuine skill result from ordinary
output that merely quotes such text. This is intentionally narrow; it is the
only text-derived path and is paired with the no-skill experiment-health
invariant in the result checker as a backstop.
"""
for text in _tool_result_texts(content):
if _SKILL_RESULT_HEADER_RE.match(text) and _SKILL_RESULT_MARKER in text.lower():
return True
if _SKILL_CONTENT_ENVELOPE_RE.match(text):
return True
return False


Expand All @@ -104,11 +120,13 @@ def is_skill_invocation_event(event: Mapping[str, Any]) -> bool:

``kind == "skill"`` is the canonical representation. Identity signals (tool
kind, tool name, or title naming ``invoke_skill`` / ``activate_skill``) are
trusted outright. Older OpenHands ACP artifacts emitted ``invoke_skill``
calls as ``kind == "other"`` with the structured tool result in ``content``;
that shape is recognized only when the tool kind is unclassified, so an
ordinary ``read`` / ``execute`` / ``search`` tool whose output happens to
quote the marker is never reclassified.
trusted outright. Harnesses that do not set the canonical kind are matched
on their own structured shape -- OpenHands legacy emits ``invoke_skill`` as
``kind == "other"`` with the tool result in ``content``, and opencode emits
``kind == "other"`` / ``title == "skill"`` with a ``<skill_content>``
envelope. Both are recognized only when the tool kind is unclassified, so an
ordinary ``read`` / ``execute`` / ``search`` tool whose title or output
happens to mention a skill is never reclassified.
"""
if event.get("type") != "tool_call":
return False
Expand All @@ -127,6 +145,13 @@ def is_skill_invocation_event(event: Mapping[str, Any]) -> bool:
if kind not in _CONTENT_SNIFFABLE_KINDS:
return False

# opencode labels the call simply ``skill``. That bare word is too generic
# to trust on a tool that already declares a real ACP kind, so unlike the
# explicit ``invoke_skill`` spellings above it is only honored for the
# unclassified kinds -- the same gate the content sniffing sits behind.
if title in _SKILL_TOOL_NAMES:
return True

return content_contains_skill_invocation_tool(event.get("content"))


Expand Down
65 changes: 65 additions & 0 deletions tests/test_skill_invocation_artifacts.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,71 @@ def test_skill_invocation_count_ignores_marker_in_nested_metadata() -> None:
assert count_skill_invocations(trajectory) == 0


def test_skill_invocation_count_accepts_opencode_skill_content_envelope() -> None:
"""opencode reports a skill call as kind="other" / title="skill" with the
skill body in a <skill_content> envelope. Neither the canonical kind nor the
OpenHands header is present, so before this shape was recognized every
opencode with-skill rollout reported n_skill_invocations=0."""
Comment on lines +134 to +137

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Name the guarded change in regression docstrings

The three newly added regression tests—this OpenCode-envelope case and the two negative cases below it—describe the behavior they protect but do not identify the PR or commit being guarded. Add the relevant PR/commit reference to each docstring so they comply with the repository's explicit regression-test convention.

AGENTS.md reference: AGENTS.md:L17-L17

Useful? React with 👍 / 👎.

trajectory = [
{
"type": "tool_call",
"tool_call_id": "call_D3Vsvu3AN2TVSjfUuJpeDJdF",
"kind": "other",
"title": "skill",
"status": "completed",
"content": [
{
"type": "content",
"content": {
"type": "text",
"text": (
'<skill_content name="polar-electrostatics-mentor">\n'
"# Skill: polar-electrostatics-mentor\n"
),
},
}
],
}
]

assert count_skill_invocations(trajectory) == 1


def test_skill_invocation_count_ignores_quoted_skill_content_envelope() -> None:
"""The <skill_content> envelope counts only when it opens the tool result.
A tool that greps for the tag is not a skill invocation."""
trajectory = [
{
"type": "tool_call",
"kind": "other",
"title": "grep skill_content trajectory.jsonl",
"content": [
{
"type": "content",
"content": {
"type": "text",
"text": 'trajectory.jsonl:8:<skill_content name="pdf">',
},
}
],
}
]

assert count_skill_invocations(trajectory) == 0


def test_skill_titled_tool_with_real_kind_is_not_a_skill_invocation() -> None:
"""title="skill" is honored only for unclassified kinds. A read/execute tool
that happens to be titled "skill" keeps its declared identity, so no-skill
rollouts cannot be contaminated by a filename."""
trajectory = [
{"type": "tool_call", "kind": "read", "title": "skill"},
{"type": "tool_call", "kind": "execute", "title": "Skill"},
]

assert count_skill_invocations(trajectory) == 0


def test_build_rollout_result_writes_skill_invocation_metric(tmp_path) -> None:
"""Guards issue #507: result.json exposes structured skill invocation counts."""
trajectory = [
Expand Down
Loading