diff --git a/src/benchflow/trajectories/metrics.py b/src/benchflow/trajectories/metrics.py index b4f8535f0..f70fe67b7 100644 --- a/src/benchflow/trajectories/metrics.py +++ b/src/benchflow/trajectories/metrics.py @@ -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 ```` 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* 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 ```` 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 @@ -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 ```` + 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 @@ -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")) diff --git a/tests/test_skill_invocation_artifacts.py b/tests/test_skill_invocation_artifacts.py index 175e30a6b..e417a953f 100644 --- a/tests/test_skill_invocation_artifacts.py +++ b/tests/test_skill_invocation_artifacts.py @@ -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 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.""" + trajectory = [ + { + "type": "tool_call", + "tool_call_id": "call_D3Vsvu3AN2TVSjfUuJpeDJdF", + "kind": "other", + "title": "skill", + "status": "completed", + "content": [ + { + "type": "content", + "content": { + "type": "text", + "text": ( + '\n' + "# Skill: polar-electrostatics-mentor\n" + ), + }, + } + ], + } + ] + + assert count_skill_invocations(trajectory) == 1 + + +def test_skill_invocation_count_ignores_quoted_skill_content_envelope() -> None: + """The 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:', + }, + } + ], + } + ] + + 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 = [