fix: detect Windows skill paths in telemetry

[dmorosanu]

Summary

Fix skill_triggered detection for Codex telemetry containing Windows paths.

Problem

Codex records skill reads inside PowerShell command strings using backslashes, sometimes retained as doubled JSON-escaped separators. The checker only recognized skills/<name>/, so a skill could be loaded correctly while the evaluation incorrectly reported observed=no. Final scoring and early-stop detection also used separate matching paths, allowing their results to diverge.

Change

Use one shared detector for final scoring and early-stop behavior. It accepts POSIX separators, Windows separators, doubled escaped separators, and overlapping repository paths such as skills/skills/<name>/.

Validation

• 127 passed: tests/test_skill_triggered.py and tests/test_early_stop.py
• Ruff lint and formatting passed
• Rechecked the four original troubleshoot trajectories: all report skill_triggered as observed=yes, score 1.0
• Fresh four-test end-to-end run: activation passed in all four scenarios

[github-actions]

Claude finished @dmorosanu's task in 1m 36s —— View job

---

Code Review in Progress

Review Checklist:

• Read .github/code_review.md for review standards
• Read CLAUDE.md for project conventions
• View full PR diff (origin/main...HEAD)
• Read complete changed files for context
• Analyze code changes for correctness
• Check cross-file consistency
• Perform "what's missing" analysis
• Provide design-level scrutiny
• Format review per code_review.md guidelines

[uipreliga]

Review: coder_eval — pr:24

Scope: pr:24 · branch codex/fix-skill-triggered-windows · 638605e · 2026-07-16T21:35Z · workflow variant

Change class: simple — localized regex change adding Windows/JSON-escaped separator support to skill-path detection in one criterion, covered by 4 new targeted tests

coder_eval is in excellent health (10/10 overall, all axes green) with only two cosmetic/low-risk issues, both isolated to the skill_triggered criterion — the one real risk is a thin false-positive coverage gap around the new Windows/backslash separator regex that could, in principle, flip a classification verdict, so the bottom line is ship-as-is after tightening that test and correcting one stale docstring.

Summary

Axis	Score	🔴	🟠	🟡	🔵	Top Issue
1. Code Quality & Style	9.9 / 10	0	0	0	1	_engaged_skill docstring is stale after Windows-separator / delegation refactor (still describes POSIX-only substring matching)
2. Type Safety	10 / 10	0	0	0	0	—
3. Test Health	9.9 / 10	0	0	0	1	New Windows/JSON-escaped separator handling has only positive tests; no negative (false-positive) coverage for the new backslash paths
4. Security	10 / 10	0	0	0	0	—
5. Architecture & Design	10 / 10	0	0	0	0	—
6. Error Handling & Resilience	10 / 10	0	0	0	0	—
7. API Surface & Maintainability	10 / 10	0	0	0	0	—
8. Evaluation Harness Quality	10 / 10	0	0	0	0	—

Overall Score: 10 / 10 · Weakest Axis: Code Quality & Style at 9.9 / 10
Totals: 🔴 0 · 🟠 0 · 🟡 0 · 🔵 2 across 8 axes.

Blockers

None.

Non-blocking, but please consider before merge

None.

Nits

1. [Axis 1] _engaged_skill docstring is stale after Windows-separator / delegation refactor (still describes POSIX-only substring matching) (src/coder_eval/criteria/skill_triggered.py:78) — This PR replaced _engaged_skill's inline body with return skill_name in _engaged_skill_names(cmd) (line 83), but left its docstring describing the old, superseded mechanism. Lines 76-82 still say detection works because paths "contain the substring skills/<skill_name>/" and "The trailing slash prevents prefix collisions" — i.e. plain POSIX substring containment. That is no longer how it works: the actual matching now lives in _engaged_skill_names via the regex _SKILL_PATH_RE = re.compile(r"(?=skills[\\/]+([A-Za-z0-9][A-Za-z0-9_-]*)[\\/]+)") (line 41), which also accepts Windows backslash separators and doubled backslashes — the very behavior this PR added. Unlike _engaged_skill_names' docstring (which was updated to mention skills\<name>\), _engaged_skill's was not, so it now both misdescribes the signal (omits Windows/backslash) and duplicates documentation that properly belongs to _engaged_skill_names. _engaged_skill also has exactly one caller (line 85 in _check_impl). Fix: trim the docstring to a one-liner deferring to _engaged_skill_names for the detection signals (or add the Windows/backslash note), and consider inlining the single call site as criterion.skill_name in _engaged_skill_names(cmd) to drop the now-redundant wrapper.
2. [Axis 3] New Windows/JSON-escaped separator handling has only positive tests; no negative (false-positive) coverage for the new backslash paths (tests/test_skill_triggered.py:161) — The regex change adds two new separator behaviors — backslash separators (skills[\\/]+) and the + quantifier for JSON-escaped skills\\<name>\\ — but every negative-case test exercises POSIX / only. test_codex_prefix_collision_guarded (line 161) asserts /Users/x/uipath/skills/skills/uipath-agents-extra/SKILL.md does NOT match uipath-agents, yet there is no Windows-separator equivalent (e.g. ...\skills\uipath-agents-extra\SKILL.md), and no test pins that an empty-name path (skills\\ / skills// with no name char) yields no match — a guard the source comment relies on (the [A-Za-z0-9] leading class). The positive Windows cases are tested (test_skill_triggered.py:136,144; test_early_stop.py:245,249) and the char-class logic is separator-agnostic and its POSIX equivalents are covered, so impact is low; add one Windows-separator prefix-collision negative and one empty-name (skills\\) rejection assertion to TestEngagedSkillNames to lock the false-positive-avoidance side of the new regex.

What's Missing

Daily/nightly:

• 🟡 The regex swap (skills/([A-Za-z0-9]...)/ → lookahead (?=skills[\\/]+([A-Za-z0-9]...)[\\/]+)) broadens what counts as a skill engagement — backslash separators, one-or-more [\\/]+ separators, and overlapping lookahead matches — yet skill_triggered is the classification criterion that feeds the nightly activation suite's accuracy / recall / F1 / confusion matrix (models/results.py _CLASSIFICATION_CRITERION_TYPES, gated by suite_thresholds and the CLI exit code). The PR states no impact on the nightly activation run; any command string containing a skills<sep>name<sep> sequence that newly matches (or a new overlap) could shift the confusion matrix / gate outcome. Unstated blast radius on the production nightly path is itself the gap. (trigger: src/coder_eval/criteria/skill_triggered.py)

Downstream consumers:

• 🔵 The boolean the broadened matcher produces flows downstream into aggregate() (classification overlay: accuracy/P/R/F1/confusion) and reports.py:763's per-skill skill_triggered rollup, then into BaseSuccessCriterion.suite_thresholds and CLI pass/fail. No regression/golden fixture pins that the broadened detection leaves those aggregate numbers unchanged for existing POSIX inputs, so a matcher over-broadening (e.g. skills////name/// via the + quantifier) would silently move suite metrics with no test catching it. Restates Axis 5: unstated nightly blast radius. (trigger: src/coder_eval/criteria/skill_triggered.py) (restates: Axis 5: nightly activation suite blast radius)

Tests:

• 🔵 The entire reason for switching from a consuming regex to a zero-width lookahead is to permit overlapping matches (.../skills/skills/<name>/... → both skills and <name>, per the new inline comment), but no positive test pins _engaged_skill_names returning that overlapping set — the behavior is only exercised indirectly through the POSIX prefix-collision negative. Add a positive assertion on the returned set for an overlapping path. (trigger: tests/test_skill_triggered.py) (restates: Axis 3: no negative/regex coverage for new separators)
• 🔵 The still-present empty-name guard ([A-Za-z0-9] leading class) that rejects skills///skills\\ is untested under the new [\\/]+ quantifier, and there is no Windows-separator prefix-collision negative mirroring test_codex_prefix_collision_guarded (e.g. ...\skills\uipath-agents-extra\SKILL.md must not match uipath-agents). Add both to TestEngagedSkillNames to lock the false-positive-avoidance side of the new regex. (trigger: tests/test_skill_triggered.py) (restates: Axis 3: no negative/false-positive coverage for backslash paths)

Harness & Lint Improvements

Static checks (lint / type):

• (none)

Harness improvements (not statically reachable):

• Add the two missing reject-side tests to TestEngagedSkillNames (a Windows-separator prefix-collision negative like '...\skills\uipath-agents-extra\SKILL.md' NOT matching 'uipath-agents', and an empty-name rejection for 'skills\'/'skills//'), then make the reject-side durable by wiring a scoped mutation-testing gate (e.g. mutmut/cosmic-ray over criteria/skill_triggered.py) into make verify so a surviving mutant that widens [\\/]+ to [/]+ or drops the + fails CI. Why not static: Line/branch coverage marks the regex 'covered' after a single positive match, so no static gate can see that its accept-side is tested while its reject-side (prefix collision + empty-name) is not; distinguishing false-positive-avoidance from positive matching requires executing the suite against deliberately mutated source, not scanning it. Prevents: Finding 2 (new backslash/JSON-escaped separator branches have only positive coverage; no negative false-positive tests).
• Fix the stale _engaged_skill docstring at source by trimming it to a one-liner that defers to _engaged_skill_names for the detection signals (single-source the Windows/backslash detection docs on the delegate), and consider inlining the single-caller wrapper as criterion.skill_name in _engaged_skill_names(cmd); treat this as a point fix, not a recurring-class prevention. Why not static: Semantic drift between a prose docstring and code behavior is not grep-/AST-detectable — no ruff, pyright, or CE rule can know that a docstring describing 'POSIX substring containment' / 'trailing slash prevents prefix collisions' contradicts a body that now delegates to a Windows-aware regex, because it requires reading and comparing the intent of both. There is likewise no durable harness/test gate for this class; a speculative 'single-call-site trivial-wrapper' CE rule was rejected for excessive false positives on legitimate wrappers. Prevents: Finding 1 (stale _engaged_skill docstring describing superseded POSIX-only substring matching).

Top 5 Priority Actions

1. Lock the false-positive side of the new skill-path regex by adding a Windows-separator prefix-collision negative and an empty-name (skills\\) rejection to TestEngagedSkillNames (tests/test_skill_triggered.py:161, guarding _SKILL_PATH_RE at src/coder_eval/criteria/skill_triggered.py:41), since an over-broad match flips a skill_triggered/classification verdict and changes a task's score and final_status for identical agent output.
2. Trim the stale POSIX-only docstring on _engaged_skill (src/coder_eval/criteria/skill_triggered.py:76-82) to defer to _engaged_skill_names for the detection signals, and inline its lone caller at line 85 as criterion.skill_name in _engaged_skill_names(cmd) to drop the now-redundant wrapper.
3. Per the CLAUDE.md "could a lint rule have prevented this" guidance, add a custom CE rule that flags a one-line delegating wrapper whose docstring still describes the delegated-away mechanism, converting this docstring-drift class into permanent enforcement.
4. Add a small cross-platform parametrized/property test over _engaged_skill_names covering POSIX /, Windows \, and JSON-doubled \\ separators so future edits to the separator regex cannot silently regress either the match or no-match sides.
5. No further remediation is warranted across axes 2 and 4-8 (all 10/10, zero findings); keep the existing suite gates and thresholds as-is and treat the codebase as green.

---

Stats: 0 🔴 · 0 🟠 · 0 🟡 · 2 🔵 across 8 axes reviewed.