Skip to content

fix(metrics): count opencode skill invocations - #939

Open
bingran-you wants to merge 1 commit into
mainfrom
bry/fix-opencode-skill-invocation-count
Open

fix(metrics): count opencode skill invocations#939
bingran-you wants to merge 1 commit into
mainfrom
bry/fix-opencode-skill-invocation-count

Conversation

@bingran-you

Copy link
Copy Markdown
Collaborator

Problem

n_skill_invocations is 0 for every opencode rollout, including ones that
demonstrably loaded a skill. opencode's ACP skill call looks like this:

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

is_skill_invocation_event misses it four ways over:

  1. kind is "other", not the canonical "skill".
  2. The event carries no tool_name / name / function key, so
    _event_tool_name returns "".
  3. The title check used an inline literal {"invoke_skill", "activate_skill"}
    which dropped the "skill" spelling that _SKILL_TOOL_NAMES — used by the
    kind check three lines above — already contains.
  4. Content sniffing only recognized the OpenHands Tool: invoke_skill header.

This matters more than a cosmetic counter: n_skill_invocations is the field
that answers "did the agent actually use the skill?", and it is the control
variable for every with-skill vs no-skill comparison. Reading a hard 0 for an
entire harness makes those comparisons unfalsifiable — you cannot tell a skill
that didn't help from a skill that was never loaded.

Fix

Recognize both opencode signals, preserving the module's existing rule that a
tool declaring a real ACP kind is never reclassified from its title or output:

  • <skill_content name="..."> counts only when it opens a structured
    tool-result text block (anchored \A, exactly as the OpenHands header is), so
    a grep whose output quotes the tag is not counted.
  • Bare title == "skill" is honored only for unclassified kinds
    (""/other/tool) — the same gate content sniffing sits behind. The
    explicit invoke_skill / activate_skill spellings stay trusted outright, so
    existing behavior is unchanged. A read or execute tool titled skill
    (a filename, say) is still not a skill invocation.

Verification

Replayed against 206 recorded opencode trajectories from a 29-task,
6-arm evaluation:

before after
counter agrees with an independent <skill_content name=...> scan 0 / 206 206 / 206
with-skill rollouts reporting a nonzero count 0 56
no-skill rollouts falsely counted 0 0

The no-skill invariant that backstops the text-derived path still holds.

New tests cover the opencode envelope, a quoted-tag negative, and a
real-kind tool titled skill. tests/test_skill_invocation_artifacts.py
passes 10/10 (7 pre-existing unchanged); 697 tests pass across the
skill/metrics/trajectory suites. ruff check and ruff format --check clean.

🤖 Generated with Claude Code

`n_skill_invocations` was 0 for every opencode rollout that used a skill,
because opencode's ACP shape matches none of the recognized signals:

    {"type": "tool_call", "kind": "other", "title": "skill",
     "content": [{"type": "content", "content": {"type": "text",
       "text": "<skill_content name=\"...\">..."}}]}

`kind` is "other", the event carries no tool-name key, the title check used
an inline `{"invoke_skill", "activate_skill"}` literal that dropped the
"skill" spelling `_SKILL_TOOL_NAMES` already had, and the content sniffer
only knew the OpenHands `Tool: invoke_skill` header. So the one metric that
answers "did the agent actually use the skill" silently read as "no" for a
whole harness -- and absent measurement is indistinguishable from a real
zero, which makes with-skill/no-skill comparisons unfalsifiable.

Recognize both opencode signals, keeping the module's existing discipline
that a tool declaring a real ACP kind is never reclassified from its title
or output text:

- `<skill_content name="...">` is matched only when it *opens* a structured
  tool-result text block (anchored `\A`, same as the OpenHands header), so a
  grep that quotes the tag is not counted.
- bare `title == "skill"` is honored only for unclassified kinds, unlike the
  explicit `invoke_skill` spellings which stay trusted outright.

Verified against 206 recorded opencode trajectories: the counter now agrees
exactly with an independent `<skill_content name=...>` scan on every one,
56 with-skill rollouts move from 0 to a true count, and no no-skill rollout
is counted -- the no-skill invariant that backstops this path still holds.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Your trial has ended. Reactivate Greptile to resume code reviews.

@bingran-you
bingran-you temporarily deployed to pypi-internal-preview July 30, 2026 04:36 — with GitHub Actions Inactive

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: be6a1a03dd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +134 to +137
"""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."""

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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant