feat: add skill-level evaluators for skill-equipped agents

[sangminwoo]

Description

A skill is an instruction file (usually SKILL.md) that the harness offers to an agent at runtime; the agent decides which, if any, to load. This adds evaluation for both halves of that behavior: which skill the agent selected, and whether it then followed the skill's instructions.

Parsing helpers (extractors/skills.py)
parse_available_skills and extract_selected_skills recover the skill signals from a trajectory. These are standalone helpers rather than a TraceExtractor level because skills have no structured representation in the trace schema: the available set arrives as harness-injected prompt text rather than a span field, and a skill load surfaces as an ordinary tool call. A load is detected either by a reserved skill-tool name plus its skill-name argument, or by a read of a known SKILL.md path. Both helpers accept a Session or a raw message list, so harnesses without a Session mapper are supported.

Evaluators

• SkillSelectionAccuracyEvaluator judges whether each invoked skill was an appropriate pick, one output per invoked skill, mirroring ToolSelectionAccuracyEvaluator. When no skill was invoked it judges the abstention itself.
• SkillInstructionFollowingEvaluator rates how fully each invoked skill's steps were followed on the framework's five-point scale, grounded in per-step covered/partial/skipped evidence.
• SkillInvoked is a deterministic presence check, the skill analogue of ToolCalled.

Prompts live in versioned template subpackages, following the existing convention. Both judges read the trajectory only, so any harness that emits one works: actual_trajectory accepts a Session or a raw message list.

Two supporting fixes
Both session mappers dropped the system prompt when building AgentInvocationSpan, so anything reading AgentInvocationSpan.system_prompt saw None. This is a bug independent of skills; it surfaced here because the harness advertises its skill catalog in the system prompt. Adds _extract_system_prompt handling both GenAI conventions (gen_ai.system_instructions and the legacy event form). One note on the OpenInference mapper: span-event message parsing is now unconditional rather than skipped when prompt and response were already recovered from smolagents attributes, because the system prompt lives in those messages. _get_messages_from_span_events memoizes per span_id, so there is no extra parsing cost.

calculate_overall_score now skips rows whose every output is labeled not_applicable, so an evaluator that cannot apply to a case does not deflate the aggregate with a placeholder 0.0. Behavior is unchanged for every existing evaluator, since none of them emit that label; two regression tests pin this.

Declares pyyaml, used to read SKILL.md frontmatter. It was already present transitively via strands-agents; this makes the direct dependency explicit using the same version bound.

Related Issues

#299

Documentation PR

N/A (README and SKILL.md updated in this PR)

Type of Change

New feature

Testing

How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.

• I ran hatch run prepare

Checklist

• I have read the CONTRIBUTING document
• I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works
• My change is focused and reasonably small; I have split unrelated work into separate PRs
• I have added any necessary tests that prove my fix is effective or my feature works
• I have updated the documentation accordingly
• I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
• My changes generate no new warnings
• Any dependent changes have been merged and published

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[Copilot]

Pull request overview

Adds first-class “skill” evaluation support to strands-evals by (1) extracting offered/selected skills from trajectories across multiple harness formats, and (2) introducing new judge + deterministic evaluators to score skill selection and adherence. It also fixes system-prompt loss in session mappers (needed because available skills are often injected into the system prompt) and adjusts report aggregation to avoid deflating overall scores with not_applicable rows.

Changes:

• Add skill parsing helpers (parse_available_skills, extract_selected_skills) that work on both Session objects and raw message lists, plus trajectory serialization with truncation.
• Add skill evaluators: SkillSelectionAccuracyEvaluator, SkillInstructionFollowingEvaluator, and deterministic SkillInvoked, with versioned prompt templates and unit tests.
• Fix mapper/system plumbing: backfill AgentInvocationSpan.system_prompt in Strands + OpenInference mappers; update overall score aggregation to exclude fully-not_applicable rows; declare pyyaml dependency.

Reviewed changes

Copilot reviewed 27 out of 28 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/strands_evals/types/test_evaluation_report.py	Adds regression tests for excluding not_applicable rows from overall score aggregation.
tests/strands_evals/mappers/test_strands_in_memory_mapper.py	Verifies Strands in-memory mapper now captures/backfills system prompt for skill catalog parsing.
tests/strands_evals/mappers/test_openinference_session_mapper.py	Verifies OpenInference mapper backfills system prompt onto agent spans regardless of span order.
tests/strands_evals/extractors/test_skills.py	New unit tests covering skill catalog parsing + invocation extraction across harness formats.
tests/strands_evals/extractors/skill_fixtures.py	Golden fixtures for cross-harness raw message shapes used by skill extractor tests.
tests/strands_evals/evaluators/test_skill_selection_accuracy_evaluator.py	New tests for skill selection judge prompt construction, looping semantics, and label/score mapping.
tests/strands_evals/evaluators/test_skill_instruction_following_evaluator.py	New tests for step-status grounding, ordinal mapping, N/A aggregation behavior, and async path.
tests/strands_evals/evaluators/deterministic/test_skill_invoked.py	New tests for deterministic skill presence evaluator.
src/strands_evals/types/evaluation_report.py	Adds calculate_overall_score to skip fully-not_applicable rows when averaging.
src/strands_evals/mappers/strands_in_memory_session_mapper.py	Extracts system prompt from GenAI conventions and backfills it into agent spans.
src/strands_evals/mappers/openinference_session_mapper.py	Ensures system prompt is parsed/cached from span events and backfilled into agent spans.
src/strands_evals/extractors/skills.py	New skill extraction/parsing module (available catalog + invoked skills + serialization).
src/strands_evals/extractors/init.py	Exposes skill extractors and serialization helpers from the extractors package.
src/strands_evals/experiment.py	Uses EvaluationReport.calculate_overall_score during async experiment reporting.
src/strands_evals/evaluators/skill_selection_accuracy_evaluator.py	New judge evaluator for per-skill selection appropriateness (and abstention correctness).
src/strands_evals/evaluators/skill_instruction_following_evaluator.py	New judge evaluator for per-skill step adherence using a five-point ordinal rubric.
src/strands_evals/evaluators/prompt_templates/skill_selection_accuracy/skill_selection_accuracy_v0.py	New prompt template for skill selection accuracy judge.
src/strands_evals/evaluators/prompt_templates/skill_selection_accuracy/init.py	Version registry for skill selection accuracy prompt templates.
src/strands_evals/evaluators/prompt_templates/skill_instruction_following/skill_instruction_following_v0.py	New prompt template for skill instruction-following judge.
src/strands_evals/evaluators/prompt_templates/skill_instruction_following/init.py	Version registry for skill instruction-following prompt templates.
src/strands_evals/evaluators/deterministic/skill_invoked.py	New deterministic evaluator that checks whether a named skill was invoked.
src/strands_evals/evaluators/deterministic/init.py	Exports SkillInvoked from deterministic evaluators.
src/strands_evals/evaluators/init.py	Exports new skill evaluators at the package top level.
SKILL.md	Documents newly added skill-level evaluators and their scoring behavior.
README.md	Adds user-facing docs and example usage for the new skill evaluators.
pyproject.toml	Declares direct pyyaml dependency (used to parse SKILL.md frontmatter).
AGENTS.md	Documents strands.Skill usage as a real SDK import used by skill extractors.

Comments suppressed due to low confidence (3)

src/strands_evals/evaluators/skill_instruction_following_evaluator.py:148

• When actual_trajectory is None, extract_selected_skills(None) returns [], so this path currently reports "no skill invoked" and (via the aggregator) a passing result. That treats missing data as a correct no-skill run. Add an explicit None check and return a not_applicable row with test_pass=False (consistent with SkillSelectionAccuracyEvaluator).

    def evaluate(self, evaluation_case: EvaluationData[InputT, OutputT]) -> list[EvaluationOutput]:
        invoked = extract_selected_skills(evaluation_case.actual_trajectory)
        if not invoked:
            return [self._not_applicable_row("no skill invoked")]

src/strands_evals/evaluators/skill_instruction_following_evaluator.py:164

• Same as the sync path: actual_trajectory=None currently falls through to "no skill invoked" (passing) because extract_selected_skills(None) returns []. Add an explicit None check and return a failing not_applicable row.

    async def evaluate_async(self, evaluation_case: EvaluationData[InputT, OutputT]) -> list[EvaluationOutput]:
        invoked = extract_selected_skills(evaluation_case.actual_trajectory)
        if not invoked:
            return [self._not_applicable_row("no skill invoked")]

src/strands_evals/evaluators/skill_selection_accuracy_evaluator.py:112

• Same issue in evaluate_async: when the available-skill catalog is missing, the judge call still happens with "(none listed)", producing a score that isn’t grounded in the offered-skill set. Consider short-circuiting to a not_applicable row when parse_available_skills(...) returns empty.

    async def evaluate_async(self, evaluation_case: EvaluationData[InputT, OutputT]) -> list[EvaluationOutput]:
        if evaluation_case.actual_trajectory is None:
            return [self._missing_trajectory_row()]
        invoked = extract_selected_skills(evaluation_case.actual_trajectory)
        evaluator_agent = Agent(model=self.model, system_prompt=self.system_prompt, callback_handler=None)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/strands_evals/evaluators/skill_instruction_following_evaluator.py:184

• SkillInstructionFollowingEvaluator.evaluate_async() also creates a new Agent per invoked skill. Reusing a single Agent instance per case avoids repeated setup and reduces latency/cost when multiple skills are invoked.

            prompt = self._build_prompt(skill, evaluation_case)
            evaluator_agent = Agent(model=self.model, system_prompt=self.system_prompt, callback_handler=None)
            result = await evaluator_agent.invoke_async(prompt, structured_output_model=SkillFollowingRating)
            rating = cast(SkillFollowingRating, result.structured_output)
            results.append(self._rating_to_output(skill, rating))

src/strands_evals/extractors/skills.py:678

• extract_selected_skills currently skips reserved skill-tool invocations when the tool call has no matching toolResult. That can undercount skill loads in trajectories where tool responses are not captured (or logs are truncated), even though the tool call itself indicates the agent attempted to load the skill. Consider counting keyed skill-tool calls as an invocation with body=None when the result is missing, while still excluding explicit failures and keeping the stricter behavior for unkeyed calls.

        if skill_name is not None:
            if matched_result is None or matched_result[0]:
                continue
            body = matched_result[1]
            if tool_name == "Skill" and body is None:

src/strands_evals/evaluators/skill_instruction_following_evaluator.py:166

• SkillInstructionFollowingEvaluator.evaluate() constructs a new Agent inside the per-skill loop. Since the agent is stateless across calls here (single prompt, structured output), this adds unnecessary overhead for multi-skill runs. Instantiate one Agent once per evaluation and reuse it across skills.

This issue also appears on line 180 of the same file.

            prompt = self._build_prompt(skill, evaluation_case)
            evaluator_agent = Agent(model=self.model, system_prompt=self.system_prompt, callback_handler=None)
            result = evaluator_agent(prompt, structured_output_model=SkillFollowingRating)
            rating = cast(SkillFollowingRating, result.structured_output)
            results.append(self._rating_to_output(skill, rating))

[jjbuck]

I noticed that this is inconsistent with the way the flow works in other evaluators like the tool selection accuracy evaluator.

In the other evaluators, e.g., https://github.com/strands-agents/evals/blob/main/src/strands_evals/evaluators/tool_selection_accuracy_evaluator.py#L50-L57, we instantiate a fresh copy of the agent inside the loop, i.e., for every tool we instantiate a fresh judge agent and invoke that fresh agent.

Here, we instantiate the judge agent (on L98) and then reuse that agent for each and every skill. I think this means we'll accumulate turns from different skill evaluations in the judge conversation, right? If that's the right reading of the current implementation, we should change it to instantiate the judge agent inside the loop.

[sangminwoo]

Thanks that's a good catch. Reusing the judge accumulated prior turns and resent the trajectory on top of that history. I fixed this by creating a fresh judge per skill while building the shared case context once. The tests now assert one judge instance and one prompt per skill.

[jjbuck]

If the intention is to exclude errors, I think there's a gap here, specifically for Strands’ AgentSkills plugin.

AgentSkills registers configured SKILL.md files and exposes a skills(skill_name=...) tool that the model uses to load one. The argument is a lookup key, not a skill definition. For example, if pdf-processing is registered but the model requests pdf-procesing (one "s", i.e., the model made a typo), Then Strands v1.42.0 returns the ordinary string "Skill 'pdf-procesing' not found...".

The tool decorator marks ordinary string returns as status="success", so _result_failed() returns false and this function treats the error message as the loaded skill body. SkillInvoked then reports that the skill was loaded, and SkillInstructionFollowingEvaluator may judge the error text as instructions. Could we add a regression fixture using this exact SDK response and either recognize it as a failed load rather than a skill body?

[sangminwoo]

Good point. AgentSkills error strings are no longer treated as skill bodies, and named refusals are recorded as failed loads. Skill 'x' activated (no instructions available) remains a successful load with no body. I added regression coverage for both raw-list and Session trajectories using the exact SDK strings, plus a fixture captured from the real AgentSkills plugin. Matching is anchored to the complete status line so normal skill instructions are not filtered.

[jjbuck]

Nit (or for follow-up): This is a pretty large monolithic file, and it mixes two different kinds of logic:

1. Understanding each harness’s raw trajectory format.
2. Deciding what counts as a skill invocation.

Those should be separate stages. Right now, Strands, Claude, Codex, Gemini, and other formats are handled through branches spread across the same functions. After parsing, their information is immediately reduced to InvokedSkill(name, body). At that point we can no longer reliably distinguish:

• A tool call that failed.
• A successful load whose body was not captured.
• Multiple loads of the same skill.
• Loads made by different agents.
• No load attempt at all.

This is why downstream code has to inspect response strings and make assumptions about message ordering.

I'd recommend that we first convert every supported trajectory format into one common representation. For example:

  SkillLoadEvent(
      name=...,
      status="attempted" | "loaded" | "failed",
      body=...,
      error=...,
      call_id=...,
      position=...,
      agent_id=...,
  )

Each harness should have a small adapter whose only job is converting its native messages into these events. The shared extractor and evaluators should operate only on SkillLoadEvent, without knowing whether the source was Strands, Claude, or Codex.

A concrete structure could be:

  extractors/skills/
      models.py
      extractor.py
      adapters/
          session.py
          strands.py
          claude.py
          codex.py

Each adapter should be tested with an authoritative fixture from that harness. serialize_trajectory should move to evaluator prompt formatting because it is unrelated to trajectory extraction. This structure keeps provider-specific changes isolated and gives each evaluator enough information to apply its own definition of selected, invoked, or followed.

[sangminwoo]

Thanks, I agree with the separation. I refactored extractors/skills.py into per-harness adapters plus a shared extractor, added the common SkillLoadEvent(name, status, body, error, call_id, position, agent_id) representation, and moved serialize_trajectory into prompt formatting. This preserves failures, uncaptured bodies, repeated loads, ordering, and agent attribution, while letting each evaluator apply its own semantics.
I also added fixtures captured from Strands AgentSkills, Google ADK, Claude Code, and Codex. They exposed and fixed previously unsupported Google GenAI and Codex rollout formats.

[Copilot]

Pull request overview

Copilot reviewed 55 out of 56 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/strands_evals/extractors/skills/adapters/registry.py:61

• _tool_call returns None immediately when an adapter matches but yields a non-str name or non-dict arguments. Because adapter shapes can overlap (and the registry is explicitly ordered for competing recognizers), this early return prevents later adapters from attempting to parse the same block, potentially dropping valid tool calls.

Instead of return None on type mismatch, continue to the next adapter and only return None after all adapters fail.

def _tool_call(block: dict[str, Any]) -> ToolCallBlock | None:
    """The tool call this block carries, or None if it is not one."""
    for adapter in _CALL_ADAPTERS:
        matched = adapter(block)
        if matched is not None:
            call_id, name, arguments = matched
            if not isinstance(name, str) or not isinstance(arguments, dict):
                return None
            return ToolCallBlock(str(call_id) if call_id is not None else None, name, arguments)
    return None

[Copilot]

Pull request overview

Copilot reviewed 55 out of 56 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/strands_evals/types/evaluation_report.py:39

• is_applicable treats an empty outputs list as applicable (True), but the docstring only discusses the “all rows are not-applicable” case. Since this method is now part of the scoring semantics (overall score / CLI averages), it would help to explicitly document the empty-list behavior so future callers don’t assume [] is dropped as vacuously “all not-applicable”.
  tests/strands_evals/evaluators/test_skill_instruction_following_evaluator.py:352
• Avoid importing production modules inside the test body. The repo’s guidance is to keep imports at module top so missing dependencies/symbols fail at collection time rather than only when a specific test executes. Here EvaluationOutput is imported inside test_aggregator_drops_not_applicable, which is easy to fix by moving it to the existing import block at the top of the file (and then using it directly).

[Copilot]

Pull request overview

Copilot reviewed 55 out of 56 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/strands_evals/types/evaluation_report.py:44

• EvaluationReport.is_applicable() currently treats any case whose outputs are all label==NOT_APPLICABLE as non-applicable, even when those rows represent an evaluation failure (test_pass=False), e.g. the skill evaluators’ “no trajectory provided” rows. This causes calculate_overall_score() (and CLI per-evaluator averages) to drop missing-data failures from the mean, which can inflate overall_score and make failures invisible in score-based dashboards.

Consider separating “declined to judge” from “failed to judge”:

• Option A: reserve NOT_APPLICABLE for true declines (with test_pass=True), and for missing-data failures leave label=None (or use a distinct label) so they remain applicable and count as 0.0.
• Option B: keep the label but refine the applicability/averaging logic (and _get_label_from_score’s N/A handling) so NOT_APPLICABLE+test_pass=False rows still count toward averages without being reverse-mapped to a verdict label.

[poshinchen]

@strandly-the-agent review the changes and tell me if there's any gap / improvements, the harness-sdk is here: https://github.com/strands-agents/harness-sdk

[strandly-the-agent]

Reviewed 789d2ff (vs merge-base 0d23f47) at poshinchen's request, cross-checked against harness-sdk's real AgentSkills plugin. This is strong work — 21 commits of real self-correction, and most of what I threw at it held up. Verdict: changes requested — 2 blockers, one of which is Copilot's objection, raised three times, still open and still correct.

✅ Verified, not asserted: pytest tests/ → 1872 passed, ruff check + mypy -p src clean. Regenerated strands_agent_skills.json with the PR's own capture script against installed strands-agents 1.50.2 → byte-identical to the checked-in fixture (and that file matches harness-sdk HEAD). Drove the real plugin's 8 result branches through both the raw-message and mapper→Session paths: they agree. XML escaping round-trips, including a </skill> injection attempt, in Python and TypeScript. from strands import Skill is valid at the declared floor strands-agents>=1.42.0. ⚠️ The branch is CONFLICTING with main.

🔴 Blockers

1. overall_score reports a failed run as 1.00. is_applicable drops a NOT_APPLICABLE row even when test_pass=False — exactly what both new evaluators emit for a missing trajectory. Copilot is right; repro inline.
2. Any read of a file named SKILL.md is recorded as an invoked skill, with no check it was ever on offer. Reading this repo's own root SKILL.md yields a phantom skill strands-evals whose "instructions" are 50k chars of contributor guide — two judge calls, two wrong scores.

🟡 7 should-fix inline. The one I'd look at first: a skill written as a decision tree may be graded on branches the run correctly never entered, and SkillStepRating.status gives the judge no way to say "not applicable to this run". Then a regex that swallows short skill bodies, the harness's metadata tail presented to the judge as SKILL.md instructions, mapper reach, and three mutation-verified test gaps. ⚪ 13 more in the appendix.

Gaps & improvements — the thing you actually asked (8 items, collapsed below). The two I'd act on: LangChain/LangGraph is in #299's harness table and shipped nowhere, and SkillLoadEvent.agent_id is permanently None because both mappers pass metadata={} while the trace carries gen_ai.agent.name — a one-line addition next to the system_prompt fix this PR already makes.

Gaps & improvements — what the harness can do vs. what these evaluators can measure

Read against harness-sdk/strands-py/src/strands/vended_plugins/skills/{agent_skills,skill}.py. Priority: 🅑 block · 🅕 follow-up issue · 🅧 dismissed.

1. 🅕 LangChain / LangGraph adapter is absent. #299's selection-signal table lists it alongside Claude Code, Strands and Codex; there is no adapter, fixture or mention. It may work incidentally via adapters/gemini.py's generic {"name":…, "args":…} fallback, but that's untested and unclaimed. Verifying six harnesses beats claiming a seventh — just say so in the PR body rather than dropping it silently.
2. 🅕 agent_id is structurally present and permanently None. SkillLoadEvent.agent_id exists and _agent_id_of reads span.metadata (extractor.py:164), but every mapper constructs ToolExecutionSpan(..., metadata={}). A real two-agent run confirms the trace does carry the attribution (gen_ai.agent.name on each invoke_agent span) and the mapper drops it. Since this PR is the one teaching the mapper to carry system_prompt, carrying gen_ai.agent.name too is a one-line addition — is it in scope?
3. 🅕 Multi-agent catalog mis-attribution. _available_from_session returns the first AgentInvocationSpan.system_prompt that has a block, so in a swarm/graph where sub-agents carry different catalogs, a later sub-agent's pick is judged against the first sub-agent's catalog. Out of #299's stated scope; worth a tracking issue before someone points this at a swarm.
4. 🅕 Missed activation — the agent that should have loaded a skill and didn't — is unmeasurable except when the case author names the skill up front via SkillInvoked. c14d88a6 removed abstention judging deliberately, and _no_invocation_row returns not-applicable with no judge call. Worth being explicit that this is the framework's existing posture, not a skills gap: ToolSelectionAccuracyEvaluator has no "should have called a tool" check either. A session-level evaluator, not a re-measurement.
5. 🅕 No timing / over-activation signal. SkillLoadEvent.position is dropped when folding to InvokedSkill, and the selection prompt explicitly tells the judge to score each invocation "on its own merits, not on whether the agent also loaded the other skills". So activating five skills to do one thing, or activating a skill after doing the work by hand, both score the same as a clean pick. extract_skill_load_events keeps position, so the raw material is there.
6. 🅕 Skill resources are invisible to the extractor. _list_skill_resources advertises scripts/, references/, assets/, and real skills routinely say "read references/x.md" — following a skill often means reading those. Grep for references/scripts/assets in extractors/skills/ returns nothing. The judge can see such a read in the serialized trajectory, but nothing links "the skill advertised it" to "the agent read it", and the truncation above is most likely to eat exactly that region.
7. 🅕 #299's "invocation rate falls out of the same helper, no judge" didn't ship. Small, and the helper it needs already exists.
8. 🅧 Dismissed after checking, so nobody re-opens them: allowed-tools enforcement (the harness itself documents it as "not yet enforced", skill.py:222 — nothing to measure); strict mode (raises at load time, before a trajectory exists); duplicate/shadowed skill names (harness dedups transparently; eval sees what the agent saw); URL-sourced skills with no filesystem path (extraction never depends on a path).

One thing you might expect me to raise and I won't: the harness keeps an exact ordered activation list in agent.state["agent_skills"]["activated_skills"] (_track_activated_skill), which looks like better ground truth than regexing prompt text. I checked — it isn't available: no mapper touches agent.state, StateEquals reads a field the task author populates, and the route would be Strands-only. The trajectory-based approach is the right generalist default here.

Questions (design / scope — non-blocking except the first)

1. Should the NOT_APPLICABLE mechanism be its own PR? It changes what overall_score means for every evaluator, in a 6,100-line skills diff. It's a genuine prerequisite (without it, a correct "no skill needed" run averages in a placeholder 0.0), so I'm not arguing for removal — I'm asking whether five new moving parts (a label value, a not_applicable property, a public is_applicable, an opt-in _aggregate_dropping_na, changed calculate_overall_score) deserve their own review and a decision record per team/API_BAR_RAISING.md. Note this repo has no needs-api-review label; the closest lever is design.
2. Is the drop-NA behaviour label-driven or opt-in? Right now it's both: calculate_overall_score drops NA cases for anyone who emits the literal label, while _aggregate_dropping_na drops NA rows only if the evaluator assigns self.aggregator. A third-party evaluator emitting the label gets half the semantics and no way to know which half.
3. EvaluationOutput(..., not_applicable=True) is silently accepted and does nothing (verified: pydantic drops the unknown kwarg, label stays None). That's the obvious path for an evaluator author. Would a real EvaluationOutput.not_applicable(reason, ...) classmethod be better — and would it let both evaluators drop their duplicated _not_applicable_row helpers?
4. Does the extractor layer need to be public at all? Six new exported names in extractors, when the three evaluators are what a user reaches for. ToolCalled's equivalent _check_session is private and exports no helper. Under 1.x this asymmetry bites: promoting a private name later is free, withdrawing a public one is a major break — and it's the difference between "we can tighten the SKILL.md regex in a patch" being yes or no. SkillLoadEvent is a 7-field NamedTuple, so exporting it freezes field order too.
5. Two words for one concept in one signature: extract_selected_skills(...) -> list[InvokedSkill]. Worth extract_invoked_skills? And SkillInvoked (evaluator) vs InvokedSkill (data) differ only in word order — both public, genuinely confusable. Not exporting InvokedSkill resolves it for free.
6. What should someone on an unrecognised harness do? _HARNESS_TOOLS is a private dict inside the library, so the answer today is "open a PR". Defensible while each adapter wants a captured fixture — but the flip side is that a user's own tool named skills taking a skill_name argument becomes a phantom skill load with the tool's output as the "skill body" (same root cause as blocker 2; cross-checking against the parsed catalog fixes both).

Appendix — non-blocking (13)

Docstring / comment drift (load-bearing here, because the asides are what a future maintainer trusts)

• models.py:41 — "the fullest body recovered wins" is not what extractor.py:145 does: the first body is kept, and a later one only replaces it when it is longer and contains the earlier text verbatim. A garbled 72-char first capture beats a legitimate 141-char re-read.
• extractor.py:1-6 — "harness-independent" overstates it: _claude_body_after and if call.name == "Skill" (extractor.py:541) are Claude-Code-specific and live here, not in adapters/claude.py. adapters/claude.py:7-10 already says so; the module docstring and _patterns.py:3-5's "adding a harness is a change to this module and its adapter, not to the traversal logic" should admit the exception.
• _patterns.py:3 cites a "design-doc B.1 table" that isn't in the repo — unresolvable for an outside contributor.
• PR body is stale in two places: it describes extractors/skills.py (now a 10-module package), and says the selection evaluator "judges the abstention itself" when no skill was invoked — c14d88a6 reversed exactly that, 7 commits and 9 days before head. README/SKILL.md/docstrings were all updated; only the body wasn't.

Small correctness / hygiene

• _available_str (skill_selection_accuracy_evaluator.py:60) renders one skill per line, so a description: | block scalar (which the harness passes through verbatim) splits across lines and the catalog stops being parseable by the judge; an empty description renders as a bare - name: , which reads like a load failure. f"- {s.name}: {s.description}" if s.description else f"- {s.name}" plus a \n→ fixes both.
• _canonical_skill_key folds data-clean and data_clean into one row. Both are loadable in the harness's default lenient mode, so two real skills can collapse to one.
• cat …/SKILL.md.bak is matched as a skill read (_SKILL_PATH has no trailing boundary), and sed -ni 's/a/b/' …/SKILL.md is reported as a read (_SED_IN_PLACE requires whitespace before -i).
• _skill_name_from_body calls Skill.from_content, which emits the harness's own logger.warning into eval output — one line per file-read event, and the "unquoted colon in description" case is common enough that the harness ships a dedicated fallback for it.
• The harness's <available_skills> block also carries <location> for every filesystem skill; AvailableSkill discards it. That element is a ready-made SKILL.md path → skill name map, i.e. most of the fix for blocker 2.
• An empty catalog is indistinguishable from an un-instrumented trajectory: the harness's literal <available_skills>\nNo skills are currently available.\n</available_skills> and "no block at all" both parse to [].
• _aggregate_dropping_na([]) returns all_pass=True where _default_aggregator([]) returns False. Unreachable today; worth aligning while the file is open.
• test_pass = normalized_score >= 0.75 diverges from the >= 0.5 used by every other five-point judge (coherence, faithfulness, response_relevance). The inline rationale is sound — prescriptive steps deserve a higher bar — but it silently changes what "pass" means for anyone comparing pass rates across judges in one report, so it belongs in the class docstring, not just a comment. Same file: the docstring claims the scale "mirrors" the existing judges; the numeric mapping does, the labels are new (and better anchored) — worth rewording.
• Fixture provenance: capture_skill_fixtures.py's docstring says the hand-written Gemini CLI / OpenAI Agents / OpenHands shapes "are marked as such" in skill_fixtures.py — they aren't. A reader of skill_fixtures.py:91-171 has no signal that those three formats were never checked against a real harness, unlike the five that were.

How this review was produced

Seven independent fresh-context passes (routing, context-build, correctness, adversarial/differential-vs-harness, API/DevX, test-quality with mutation testing, LLM-context, docs-accuracy, issue-alignment), then de-duplicated and severity-gated on reachability. Repros were run against installed strands-agents 1.50.2 with the real AgentSkills plugin; no live model was invoked anywhere, so every judge-behaviour claim is grounded in the prompt text and the output schema rather than measured — I've flagged that where it matters. Then an independent audit pass re-verified every load-bearing claim and sent six back for correction, which is why two things you might expect to see aren't here: a pass reported that SkillFollowingRating.coverage feeds the score (it doesn't — it only appears in reason), and my first draft of blocker 2 illustrated itself with a repro that didn't reproduce. Both were fixed rather than published.

I'm an experimental agent — treat this as solid work for a human to approve, not a gate. Push back on anything that doesn't hold up.

[sangminwoo]

@strandly-the-agent Thanks, I went through the blockers and should-fix items individually. Most are now addressed, and two I’m leaving out with rationale.

Fixed

• is_applicable blocker: Confirmed. Applicability now distinguishes declining to judge (test_pass=True, droppable) from failing to judge (test_pass=False, retained), so a missing trajectory can no longer disappear from overall_score. CLI and telemetry use the same predicate.
• \s* regex bug: Confirmed. The regexes could swallow multiline skill bodies and even misclassify a body beginning with refusal-like text as a failed load. Both are now line-bounded.
• Harness metadata leakage: Reproduced with the AgentSkills fixture. The appended Location/resource metadata is now removed by field name rather than splitting on bare ---, which can be valid skill Markdown.
• Unnamed SKILL.md reads: Reads that would resolve to . or SKILL.md no longer produce skill events.
• Truncation guidance: The prompt now tells the judge that relevant evidence may fall inside the omitted middle section.
• Mutation-test gaps: Replaced both tests that were passing for the wrong reason and verified that the intended mutations now fail.

Decision-tree grading

I also live-checked this against Sonnet 5 five times. The judge did mark untaken branches as skipped, as predicted, but still returned Fully Followed / 1.0 every time. So the proposed 0.5 scoring failure does not reproduce.

The actual issue was the diagnostic coverage in reason: it reported 0.60 for a correct conditional execution. A prompt guideline fixes that to 1.00 / 5/5 in 5/5 runs without changing the rating, so I did not add another status enum.

Not taking

• Require file-read skills to appear in the catalog: I don’t think we can make this a hard requirement because Codex does not emit <available_skills> at all. Doing so would disable its file-read detection entirely. I kept the safe subset of the fix above. The residual false-positive case is real, but across 3,379 Tessl + SkillsBench trajectories from three agent models I found 0 false positives. I also documented that callers should inspect parse_available_skills before relying on the score. I think fully solving this needs a catalog-optional design rather than gating on catalog presence.
• Span-cache re-keying: Confirmed that this behavior predates and is untouched by this PR. I’d prefer to keep it as a separate change rather than expand this PR further.