From d1cb68512d4f218d5c05ded8aa812bf8cf36ed29 Mon Sep 17 00:00:00 2001 From: anonymous Date: Wed, 22 Jul 2026 11:41:14 -0700 Subject: [PATCH 1/3] fix(early-stop): decide skill activation on the tool call, not its result Armed skill-activation rows that clearly trigger a skill were not early-stopping and burned their full turn budget. Two facts combined to break the stop at scale: 1. The live watcher evaluated armed criteria on the tool *result* (`ToolEndEvent`). When a turn is cut short (e.g. a timeout), the `Skill` tool call is left unresolved: the deadline check discards the pending result and `finalize` force-closes the call as UNRESOLVED, which the watcher (correctly) ignores. So the decision, though fully determined by the call itself, was never observed and the run kept going. 2. The final `skill_triggered` scorer used "any engagement" while the live verdict used "first engagement". Because the run kept going, the agent could engage additional skills, and each extra engagement was counted as a false positive against its own criterion. This change: - (early_stop) Evaluates armed criteria on the tool *call* (`ToolStartEvent`) in addition to the result. For an observable criterion the verdict is fully determined by the call's inputs (which skill / which command), so the watcher latches the instant the call is dispatched. The agent polls `should_stop` immediately after dispatching each message, so the loop breaks before a cut-short turn can strip the result. The in-flight call (not yet in the collector, which reduces commands from `ToolEndEvent`) is appended to the partial trajectory and reported at `tool_call_index + 1`; the counter still advances on the resolved `ToolEndEvent`. UNRESOLVED ends remain ignored, so an orphaned tool closed post-loop can never latch a false stop. - (skill_triggered) Scores the final check on the FIRST engaged skill, matching the live verdict, via a shared `_first_engaged_skill_names` helper. A second skill invoked alongside or after the first is no longer counted as a competing activation, so it is not a false positive. The live verdict and the authoritative score now agree by construction, whether or not the run stopped early. Removes the now-unused `_engaged_skill` helper. - (formatting) Skips the SDK `RateLimitEvent` (an out-of-band throttling notice) in `format_messages` alongside `StreamEvent`, so it no longer logs a spurious "unhandled SDK message type" warning. Cosmetic; does not affect scoring. Adds unit coverage for the tool-call decision (call fires before/without a result, latches before a later UNRESOLVED end, index accounting), the orchestrator cut on a resultless call, first-engagement scoring including the parallel-call case, and the rate-limit event skip. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/coder_eval/criteria/skill_triggered.py | 78 +++++++++-------- src/coder_eval/formatting.py | 9 +- src/coder_eval/orchestration/early_stop.py | 98 +++++++++++++++------- tests/test_agent.py | 12 +++ tests/test_early_stop.py | 71 ++++++++++++++++ tests/test_skill_triggered.py | 36 ++++++++ 6 files changed, 236 insertions(+), 68 deletions(-) diff --git a/src/coder_eval/criteria/skill_triggered.py b/src/coder_eval/criteria/skill_triggered.py index 3d1aaf65..3e35d800 100644 --- a/src/coder_eval/criteria/skill_triggered.py +++ b/src/coder_eval/criteria/skill_triggered.py @@ -44,16 +44,22 @@ def _engaged_skill_names(cmd: CommandTelemetry) -> set[str]: """All skill names engaged by ONE command, agent-agnostically (any-skill). - Generalizes ``_engaged_skill`` (which asks about one named skill) so a - live verdict can detect a *competing* skill engagement. Collects: - - - the Claude ``Skill`` tool call's ``skill`` parameter, namespace-stripped - via ``.split(":")[-1]`` (the SDK reports ``plugin:skill-name``); and - - every ``skills//`` or ``skills\\\\`` path segment appearing - in any string parameter (the Codex / file-read signal — repo layout or - the ``.agents/skills`` sandbox symlink). - - Returns the (possibly empty) set of engaged skill names for this command. + Detects both engagement signals so the criterion scores identically across + agents, and returns the (possibly empty) set of engaged skill names: + + - Claude: an explicit ``Skill`` tool call carries the skill in + ``parameters['skill']``, optionally namespaced (e.g. + ``plugin:uipath-agents``); the namespace is stripped via ``.split(":")[-1]``. + - Codex (and any non-Claude agent): no ``Skill`` tool exists, so a skill is + engaged by reading its files off disk via shell. Both the repo layout + (``.../skills//...``) and the sandbox symlink + (``.agents/skills//...``) contain the substring ``skills//``, + matched here in any string parameter (Bash ``parameters['command']`` or a + file-path parameter). The trailing separator required by ``_SKILL_PATH_RE`` + prevents prefix collisions (``uipath-agents`` vs ``uipath-agents-foo``). + + Returning the full set (rather than a single-skill yes/no) lets callers detect + a *competing* skill engagement. """ names: set[str] = set() if cmd.tool_name == "Skill": @@ -66,21 +72,22 @@ def _engaged_skill_names(cmd: CommandTelemetry) -> set[str]: return names -def _engaged_skill(cmd: CommandTelemetry, skill_name: str) -> bool: - """True when one command engaged ``skill_name`` — agent-agnostically. +def _first_engaged_skill_names(turn_records: list[TurnRecord]) -> set[str]: + """Skills engaged by the FIRST command that engages any skill (else empty). - Claude: an explicit ``Skill`` tool call carries the skill in - ``parameters['skill']`` (optionally namespaced, e.g. ``plugin:uipath-agents``). - - Codex (and any non-Claude agent): no ``Skill`` tool exists, so the skill is - engaged by reading its files off disk via shell. Both the repo layout - (``.../skills//...``) and the sandbox symlink - (``.agents/skills//...``) contain the substring - ``skills//``, which appears in the recorded command string - (Bash ``parameters['command']``) or a file-path parameter. The trailing - slash prevents prefix collisions (``uipath-agents`` vs ``uipath-agents-foo``). + Activation measures which skill the agent selects *first*, so scoring keys off + the first engaging command rather than "any command anywhere in the run". This + keeps the final check and the live verdict consistent and prevents a later, + incidental engagement (or a second skill invoked alongside the first) from + being counted as a competing activation and mis-scored as a false positive. + Commands are scanned in ``sequence_number`` order (turn records preserve it). """ - return skill_name in _engaged_skill_names(cmd) + for turn in turn_records: + for cmd in turn.commands: + engaged = _engaged_skill_names(cmd) + if engaged: + return engaged + return set() @register_criterion @@ -116,9 +123,10 @@ def _check_impl( error="turn_records not provided to checker", ) - triggered: bool = any( - _engaged_skill(cmd, criterion.skill_name) for turn in turn_records for cmd in turn.commands - ) + # First-engagement policy (mirrors ``live_verdict``): the run is scored on + # the FIRST skill the agent engages, so a second skill invoked alongside or + # after it is not counted as a competing activation. + triggered: bool = criterion.skill_name in _first_engaged_skill_names(turn_records) expected_yes: bool = criterion.expected_skill == criterion.skill_name score = 1.0 if triggered == expected_yes else 0.0 observed = _YES if triggered else _NO @@ -152,18 +160,16 @@ def live_verdict( This covers the "wrong skill loads" case (a positive wrong signal) and negative rows (``expected_skill == ""`` -> any engagement of the target - fails that criterion). It is a *policy* made consistent by stopping: the - trajectory is frozen at the deciding event, so the standard checker on the - truncated trajectory agrees with this verdict by construction. + fails that criterion). ``_check_impl`` applies the SAME first-engagement + policy on the full trajectory, so the live verdict and the authoritative + score agree by construction — whether or not the run stopped early. """ + engaged = _first_engaged_skill_names(turn_records) + if not engaged: + return "undecided" expected_yes = criterion.expected_skill == criterion.skill_name - for turn in turn_records: - for cmd in turn.commands: - engaged = _engaged_skill_names(cmd) - if engaged: - observed_yes = criterion.skill_name in engaged - return "pass" if observed_yes == expected_yes else "fail" - return "undecided" + observed_yes = criterion.skill_name in engaged + return "pass" if observed_yes == expected_yes else "fail" def aggregate( self, diff --git a/src/coder_eval/formatting.py b/src/coder_eval/formatting.py index 26c296ff..15685eeb 100644 --- a/src/coder_eval/formatting.py +++ b/src/coder_eval/formatting.py @@ -80,9 +80,12 @@ def format_messages( continue type_name = type(msg).__name__ - # StreamEvent is a known SDK type used for token-delta capture - # elsewhere; don't surface it as an "unhandled" warning here. - if type_name == "StreamEvent": + # Known, non-transcript SDK types: StreamEvent carries token deltas + # (captured elsewhere) and RateLimitEvent is an out-of-band throttling + # notice the SDK interleaves into the stream. Neither is transcript + # content, so skip both rather than surfacing an "unhandled" warning. + # Matched by name (not import) to stay robust across SDK versions. + if type_name in ("StreamEvent", "RateLimitEvent"): continue if type_name not in warned_unknown_types: warned_unknown_types.add(type_name) diff --git a/src/coder_eval/orchestration/early_stop.py b/src/coder_eval/orchestration/early_stop.py index 7891fb23..7a4a61f3 100644 --- a/src/coder_eval/orchestration/early_stop.py +++ b/src/coder_eval/orchestration/early_stop.py @@ -9,13 +9,21 @@ never a silent no-op. * ``EarlyStopWatcher`` — the runtime observer. A ``StreamCallback`` composed into the agent's event stream that maintains its own ``EventCollector``, - evaluates every armed criterion's ``live_verdict`` on each tool completion, - applies the stop rule, and exposes ``should_stop()`` (the cooperative - interrupt the agent polls) plus ``info`` (the ``EarlyStopInfo`` the + evaluates every armed criterion's ``live_verdict`` on each tool *call* (and on + its result), applies the stop rule, and exposes ``should_stop()`` (the + cooperative interrupt the agent polls) plus ``info`` (the ``EarlyStopInfo`` the orchestrator records). Fail-open: a raising ``live_verdict`` disarms the watcher and degrades to a full run — a verdict bug can never cause a *false* early stop. +Deciding on the tool *call* (``ToolStartEvent``), not the result, is what makes +the stop robust: for an observable criterion the verdict is fully determined by +the call's inputs (which skill / which command), so the watcher can latch the +instant the call is dispatched — before a cut-short turn (e.g. a timeout) can +strip the result and leave the call unresolved. The agent polls ``should_stop`` +immediately after dispatching each message, so a latch on the call breaks the +loop before the result message is ever pulled. + Live verdicts only *trigger* the stop; the authoritative scores always come from the standard ``check_all`` on the frozen trajectory after the cut. """ @@ -28,12 +36,19 @@ from coder_eval.models import EarlyStopInfo, EarlyStopReason from coder_eval.streaming.collector import EventCollector -from coder_eval.streaming.events import AgentStartEvent, StreamEvent, ToolEndEvent, ToolEndStatus, TurnStartEvent +from coder_eval.streaming.events import ( + AgentStartEvent, + StreamEvent, + ToolEndEvent, + ToolEndStatus, + ToolStartEvent, + TurnStartEvent, +) if TYPE_CHECKING: from coder_eval.criteria.base import BaseCriterion, LiveVerdict - from coder_eval.models import BaseSuccessCriterion, TaskDefinition + from coder_eval.models import BaseSuccessCriterion, CommandTelemetry, TaskDefinition # Armed pair the watcher holds: (criterion model, its checker). Lives in the # TYPE_CHECKING block (only annotations reference it, and those are lazy under @@ -210,25 +225,35 @@ def for_task(cls, task: TaskDefinition) -> EarlyStopWatcher: # --- StreamCallback -------------------------------------------------- # def on_event(self, event: StreamEvent) -> None: - """Forward the event to the internal collector; evaluate on tool completions. + """Forward the event to the internal collector; evaluate on each tool call. Short-circuits once the decision is latched (fired or disarmed). Counts - ``TurnStartEvent`` for ``sdk_turn_index`` and ``ToolEndEvent`` for the - 1-based ``tool_call_index``, and stamps the wall-clock origin at the - FIRST ``AgentStartEvent`` only (a retry's second AgentStart does not + ``TurnStartEvent`` for ``sdk_turn_index`` and each dispatched tool call + for the 1-based ``tool_call_index``, and stamps the wall-clock origin at + the FIRST ``AgentStartEvent`` only (a retry's second AgentStart does not reset it). - UNRESOLVED tool ends are ignored entirely (not collected, counted, or - evaluated). ``_ClaudeTurnState.finalize`` force-closes orphaned tools as - UNRESOLVED *after* the message loop has ended and the terminal status is - already chosen (COMPLETED / TIMEOUT / crash) — those are not live tool - activity and must not trip the stop rule, or a run that ran to completion - (or timed out / crashed) would latch a false early stop (e.g. an - unresolved ``Skill`` call counts as engagement because ``skill_triggered`` - ignores ``result_status``). A legitimate stop only ever fires on an - OBSERVED tool result during the loop, so dropping unresolved ends can - never suppress a real stop; it also keeps a crashed attempt's orphan tools - out of the retry-persistent partial trajectory. + The decision is evaluated on the tool *call* (``ToolStartEvent``): for an + observable criterion the verdict is fully determined by the call's inputs, + so latching here lets the agent's post-dispatch ``should_stop`` poll break + the loop before a cut-short turn can strip the result. The call is not in + the collector yet (it reduces commands from ``ToolEndEvent``), so it is + passed to ``_evaluate`` as the in-flight command, reported at + ``tool_call_index + 1`` (it has no ``ToolEndEvent`` to count yet). The + matching ``ToolEndEvent`` still evaluates, which covers a verdict that only + becomes decidable once the result is known and is a no-op once a call has + already latched the stop. ``tool_call_index`` is incremented on the + resolved ``ToolEndEvent`` so it stays a count of completed tool calls. + + UNRESOLVED tool ends are ignored entirely (not counted or evaluated). + ``_ClaudeTurnState.finalize`` force-closes orphaned tools as UNRESOLVED + *after* the message loop has ended and the terminal status is already + chosen (COMPLETED / TIMEOUT / crash) — those are not live tool activity + and must not trip the stop rule, or a run that ran to completion (or timed + out / crashed) without a real, in-loop decision would latch a false early + stop. A legitimate stop always fires on the in-loop call, so dropping + unresolved ends can never suppress a real stop; it also keeps a crashed + attempt's orphan tools out of the retry-persistent partial trajectory. """ if self._info is not None or self._disarmed: return @@ -237,13 +262,19 @@ def on_event(self, event: StreamEvent) -> None: self._started_monotonic = time.monotonic() elif isinstance(event, TurnStartEvent): self._sdk_turn_index += 1 + elif isinstance(event, ToolStartEvent): + # Decide on the call, evaluating with it appended as the in-flight + # command (it has no ToolEnd to count yet, so report it as +1). + self._evaluate(in_flight=event.tool) + return elif isinstance(event, ToolEndEvent): if event.status == ToolEndStatus.UNRESOLVED: return self._tool_call_index += 1 - self._collector.on_event(event) - if isinstance(event, ToolEndEvent): + self._collector.on_event(event) self._evaluate() + return + self._collector.on_event(event) def should_stop(self) -> bool: """The cooperative interrupt the agent polls after each dispatched message.""" @@ -261,8 +292,17 @@ def disarmed(self) -> bool: # --- Stop rule -------------------------------------------------- # - def _evaluate(self) -> None: - records = [self._collector.build_turn_record()] + def _evaluate(self, in_flight: CommandTelemetry | None = None) -> None: + record = self._collector.build_turn_record() + if in_flight is not None: + # The in-flight call has no ToolEnd yet, so the collector (which + # reduces commands from ToolEnd) has not captured it. Append it and + # re-sort by sequence so the verdict sees it in first-engagement order. + record.commands = sorted([*record.commands, in_flight], key=lambda c: c.sequence_number) + records = [record] + # An in-flight call has not been counted by a ToolEnd yet, so report it as + # the next (1-based) tool call. + tool_call_index = self._tool_call_index + (1 if in_flight is not None else 0) verdicts: list[LiveVerdict] = [] for criterion, checker in self._armed: try: @@ -283,7 +323,7 @@ def _evaluate(self) -> None: # whose stop_when permits fail decides the run. for (criterion, _checker), verdict in zip(self._armed, verdicts, strict=True): if verdict == "fail" and criterion.stop_when in ("fail", "decided"): - self._fire(EarlyStopReason.CRITERION_FAILED, criterion) + self._fire(EarlyStopReason.CRITERION_FAILED, criterion, tool_call_index=tool_call_index) return # Pass-stop: EVERY armed criterion live-passes AND each permits pass. @@ -296,13 +336,13 @@ def _evaluate(self) -> None: for (criterion, _checker), verdict, prev in zip(self._armed, verdicts, self._prev_verdicts, strict=True): if verdict != prev: deciding = criterion - self._fire(EarlyStopReason.CRITERION_PASSED, deciding) + self._fire(EarlyStopReason.CRITERION_PASSED, deciding, tool_call_index=tool_call_index) return # No stop this round — record the verdicts so the next round can detect flips. self._prev_verdicts = verdicts - def _fire(self, reason: EarlyStopReason, criterion: BaseSuccessCriterion) -> None: + def _fire(self, reason: EarlyStopReason, criterion: BaseSuccessCriterion, *, tool_call_index: int) -> None: elapsed = 0.0 if self._started_monotonic is not None: elapsed = max(time.monotonic() - self._started_monotonic, 0.0) @@ -313,7 +353,7 @@ def _fire(self, reason: EarlyStopReason, criterion: BaseSuccessCriterion) -> Non deciding_criterion_description=criterion.description, armed_criteria=[f"{c.type}: {c.description}" for c, _ in self._armed], sdk_turn_index=self._sdk_turn_index, - tool_call_index=self._tool_call_index, + tool_call_index=tool_call_index, elapsed_seconds=elapsed, turns_remaining_at_stop=turns_remaining, ) @@ -323,6 +363,6 @@ def _fire(self, reason: EarlyStopReason, criterion: BaseSuccessCriterion) -> Non reason.value, criterion.type, self._sdk_turn_index, - self._tool_call_index, + tool_call_index, elapsed, ) diff --git a/tests/test_agent.py b/tests/test_agent.py index dcf36b9b..2e4f7aaa 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -671,6 +671,18 @@ class BareMessage: assert "[ASSISTANT] Second response" in formatted assert formatted.count("[ASSISTANT]") == 2 + # Test 12: RateLimitEvent — an out-of-band throttling notice the SDK + # interleaves into the stream. It is not transcript content, so it is skipped + # (matched by class name) and does NOT surface as an "unhandled" tag/warning. + class RateLimitEvent: + pass + + formatted = agent._format_messages([RateLimitEvent()]) + assert formatted == "[No output]" + # It also does not crowd out real content when interleaved. + formatted = agent._format_messages([RateLimitEvent(), _make_assistant("still here")]) + assert formatted == "[ASSISTANT] still here" + def test_format_messages_system_message_subclasses_are_filtered(): """Regression: SystemMessage SUBCLASSES (TaskStartedMessage, etc.) must diff --git a/tests/test_early_stop.py b/tests/test_early_stop.py index 33f6a694..e76fa0cc 100644 --- a/tests/test_early_stop.py +++ b/tests/test_early_stop.py @@ -70,6 +70,7 @@ AgentStartEvent, ToolEndEvent, ToolEndStatus, + ToolStartEvent, TurnEndStatus, TurnStartEvent, ) @@ -192,6 +193,18 @@ def _tool_end(cmd: CommandTelemetry) -> ToolEndEvent: return ToolEndEvent(task_id="t", tool=cmd) +def _skill_start(skill: str, *, tool_id: str = "sk-1", sequence_number: int = 0) -> ToolStartEvent: + """A Skill ToolStart (the tool CALL) engaging ``skill`` — no result yet.""" + cmd = CommandTelemetry( + tool_name="Skill", + tool_id=tool_id, + timestamp=_TS, + parameters={"skill": skill}, + sequence_number=sequence_number, + ) + return ToolStartEvent(task_id="t", tool=cmd) + + def _skill_events(skill: str, *, tool_id: str = "sk-1") -> list[Any]: """AgentStart + TurnStart + a Skill ToolEnd engaging ``skill``.""" return [_agent_start(), _turn_start(), _tool_end(_skill_cmd(skill, tool_id=tool_id))] @@ -998,6 +1011,47 @@ def test_decision_latched_after_fire(self) -> None: assert watcher.info is not None assert watcher.info.reason == EarlyStopReason.CRITERION_PASSED + def test_tool_call_fires_before_result(self) -> None: + # The decision latches on the tool CALL (ToolStartEvent): a Skill call + # whose result never arrives (a cut-short turn would strip it) still stops. + # No ToolEndEvent is ever fed. + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + _feed(watcher, [_agent_start(), _turn_start(), _skill_start("date-teller")]) + assert watcher.should_stop() is True + assert watcher.info is not None + assert watcher.info.reason == EarlyStopReason.CRITERION_PASSED + # The in-flight call reports as the 1st tool call even without a ToolEnd. + assert watcher.info.tool_call_index == 1 + + def test_tool_call_wrong_skill_fail_fires(self) -> None: + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + _feed(watcher, [_agent_start(), _turn_start(), _skill_start("weather-teller")]) + assert watcher.info is not None + assert watcher.info.reason == EarlyStopReason.CRITERION_FAILED + + def test_tool_call_latches_before_unresolved_end(self) -> None: + # The call fires the stop in-loop; a later finalize() UNRESOLVED end for + # the SAME call is short-circuited (decision already latched) — no relabel, + # no double count. + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + _feed(watcher, [_agent_start(), _turn_start(), _skill_start("date-teller", tool_id="sk-1")]) + fired = watcher.info + _feed(watcher, [_unresolved_skill_end("date-teller", tool_id="sk-1")]) + assert watcher.info is fired + assert watcher.info is not None + assert watcher.info.tool_call_index == 1 + + def test_tool_call_index_counts_prior_resolved_calls(self) -> None: + # A prior resolved, non-deciding tool is counted at its ToolEnd; the + # deciding in-flight call is then reported as the next (2nd) call. + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + prior = _cmd("Bash", {"command": "ls"}) # not a skill engagement + _feed(watcher, [_agent_start(), _turn_start(), _tool_end(prior)]) + assert watcher.info is None + _feed(watcher, [_skill_start("date-teller", tool_id="sk-1", sequence_number=1)]) + assert watcher.info is not None + assert watcher.info.tool_call_index == 2 + # --------------------------------------------------------------------------- # # Phase 3: Orchestrator wiring @@ -1189,6 +1243,23 @@ async def test_completed_run_with_orphan_tool_not_early_stopped(self, tmp_path) assert agent.delivered == 3 # never stopped: the full stream was consumed assert result.all_criteria_passed(self._criteria()) is False + async def test_tool_call_cut_without_tool_end(self, tmp_path) -> None: + # End-to-end: the deciding Skill CALL (a ToolStart with no ToolEnd) cuts + # the stream and records an early stop — the case that would otherwise run + # to the turn cap when a cut-short turn strips the result. + events = [_agent_start(), _turn_start(), _skill_start(self._SKILL), _turn_start()] + result, agent = await _run_wiring( + criteria=self._criteria(), + events=events, + scores=[1.0, 0.0], + stop_early=True, + tmp_path=tmp_path, + ) + assert result.early_stop is not None + assert result.early_stop.reason == EarlyStopReason.CRITERION_PASSED + # Cut at the ToolStart: the trailing turn_start is never delivered. + assert agent.delivered == 3 + async def test_fail_open_wiring_degrades_to_full_run(self, tmp_path) -> None: with patch.object(SkillTriggeredChecker, "live_verdict", side_effect=RuntimeError("boom")): result, _agent = await _run_wiring( diff --git a/tests/test_skill_triggered.py b/tests/test_skill_triggered.py index ea1a8168..d2332dd0 100644 --- a/tests/test_skill_triggered.py +++ b/tests/test_skill_triggered.py @@ -98,6 +98,42 @@ def test_no_turn_records_returns_base_result_with_error(self) -> None: assert result.error is not None +class TestSkillTriggeredFirstEngagement: + """First-engagement scoring: only the FIRST skill the agent engages is scored. + + A second skill invoked alongside or after the first is not counted as a + competing activation, so it is not a false positive against its own criterion. + """ + + def _admin_platform(self) -> list[CommandTelemetry]: + # The agent engages uipath-admin FIRST, then uipath-platform. + return [ + _cmd("Skill", {"skill": "uipath-admin"}, tool_id="s1"), + _cmd("Skill", {"skill": "uipath-platform"}, tool_id="s2"), + ] + + def test_first_skill_is_true_positive(self) -> None: + # GT=uipath-admin; admin engaged first -> observed=yes, expected=yes. + result = _check(expected_skill="uipath-admin", skill_name="uipath-admin", commands=self._admin_platform()) + assert result.observed_label == "yes" and result.score == 1.0 + + def test_second_skill_not_false_positive(self) -> None: + # Same run scored for the uipath-platform criterion: platform is NOT the + # first engagement -> observed=no; expected=no (GT is admin) -> pass, no FP. + result = _check(expected_skill="uipath-admin", skill_name="uipath-platform", commands=self._admin_platform()) + assert result.observed_label == "no" and result.score == 1.0 + + def test_wrong_skill_first_still_penalized(self) -> None: + # If the WRONG skill engages first, the GT criterion correctly fails — + # first-engagement does not mask a genuine misfire. + commands = [ + _cmd("Skill", {"skill": "uipath-platform"}, tool_id="s1"), + _cmd("Skill", {"skill": "uipath-admin"}, tool_id="s2"), + ] + result = _check(expected_skill="uipath-admin", skill_name="uipath-admin", commands=commands) + assert result.observed_label == "no" and result.score == 0.0 + + class TestSkillTriggeredCodex: """Codex has no ``Skill`` tool — it engages a skill by reading its files via shell. From 0b093b1f58c7349ddb6074da6870bd313c9bceca Mon Sep 17 00:00:00 2001 From: mohsen-uipath <> Date: Wed, 22 Jul 2026 19:02:11 -0700 Subject: [PATCH 2/3] fix(early-stop): latch skill activation on any engagement, not first The skill_triggered criterion decided activation on the FIRST skill the agent engaged, so a row failed when the agent touched a wrong skill before engaging the expected one -- even though reading a SKILL.md is comparison, not commitment. Decide on ANY engagement instead: - _check_impl and live_verdict now share _all_engaged_skill_names (the union across the whole trajectory). A positive criterion (skill_name == expected_skill) passes iff the expected skill is engaged anywhere in the run (recall); a distractor/negative criterion fails on any engagement of its skill (precision), so an extra off-target engagement lands on its own confusion cell rather than opening a precision hole. Negatives (expected_skill == "") still fail on any engagement of the target skill. - Engagement is monotonic, so the live latch and the frozen-trajectory score agree by construction. - live_decidable_polarities now narrows per instance: a positive can only live-pass, a distractor/negative only live-fail. Arming a positive with fail (or either with decided) is rejected at resolution instead of silently degrading to a full run. Also confirms the early-stop watcher latches on a file-read engagement (Read/Grep of skills//...), not only a Claude Skill tool call, so early-stop fires for non-Claude agents. Adds coverage for the any-engagement scoring, stacked recall/precision on one trajectory, per-instance decidability, and the file-read latch. --- src/coder_eval/criteria/skill_triggered.py | 117 ++++++++----- src/coder_eval/orchestration/early_stop.py | 5 +- tests/test_early_stop.py | 181 +++++++++++++++------ tests/test_skill_triggered.py | 54 ++++-- 4 files changed, 254 insertions(+), 103 deletions(-) diff --git a/src/coder_eval/criteria/skill_triggered.py b/src/coder_eval/criteria/skill_triggered.py index 3e35d800..999d8ce5 100644 --- a/src/coder_eval/criteria/skill_triggered.py +++ b/src/coder_eval/criteria/skill_triggered.py @@ -72,22 +72,31 @@ def _engaged_skill_names(cmd: CommandTelemetry) -> set[str]: return names -def _first_engaged_skill_names(turn_records: list[TurnRecord]) -> set[str]: - """Skills engaged by the FIRST command that engages any skill (else empty). - - Activation measures which skill the agent selects *first*, so scoring keys off - the first engaging command rather than "any command anywhere in the run". This - keeps the final check and the live verdict consistent and prevents a later, - incidental engagement (or a second skill invoked alongside the first) from - being counted as a competing activation and mis-scored as a false positive. - Commands are scanned in ``sequence_number`` order (turn records preserve it). +def _all_engaged_skill_names(turn_records: list[TurnRecord]) -> set[str]: + """Union of every skill engaged anywhere in the trajectory (any-engagement). + + Activation is scored on whether a skill was engaged *at all* during the run, + not on which skill was engaged first. This has two consequences that the + first-engagement policy could not express: + + - **Recall (the positive criterion).** A row that engages the wrong skill + before eventually engaging the expected one is still credited for the + expected skill — reading a ``SKILL.md`` to compare candidates is + exploration, not commitment, so an earlier wrong touch must not fail the + row. + - **Precision (the distractor/negative criteria).** An unrelated skill + engaged *anywhere* is counted against its own criterion, so a positive row + that also fires an off-target skill (and a negative row that fires any + target skill) is penalized on that skill's confusion cell. + + Order is irrelevant to a set union; the scan is left in ``sequence_number`` + order purely for determinism. """ + names: set[str] = set() for turn in turn_records: for cmd in turn.commands: - engaged = _engaged_skill_names(cmd) - if engaged: - return engaged - return set() + names.update(_engaged_skill_names(cmd)) + return names @register_criterion @@ -101,8 +110,11 @@ class SkillTriggeredChecker(BaseCriterion[SkillTriggeredCriterion]): criterion_type = "skill_triggered" # Observable mid-run: a Skill tool call (or a skill file read) is a positive - # event in the live stream, so both polarities are decidable the moment the - # agent first engages ANY skill (the first-engagement policy in live_verdict). + # event in the live stream. The TYPE can decide either polarity — a positive + # criterion live-passes when its expected skill is engaged, a + # distractor/negative one live-fails when its (wrong) skill is engaged — but + # any single INSTANCE decides only one of the two; see + # ``live_decidable_polarities``. live_stop_polarities: ClassVar[frozenset[str]] = frozenset({"pass", "fail"}) def _check_impl( @@ -123,10 +135,13 @@ def _check_impl( error="turn_records not provided to checker", ) - # First-engagement policy (mirrors ``live_verdict``): the run is scored on - # the FIRST skill the agent engages, so a second skill invoked alongside or - # after it is not counted as a competing activation. - triggered: bool = criterion.skill_name in _first_engaged_skill_names(turn_records) + # Any-engagement policy (mirrors ``live_verdict``): the row is scored on + # whether this skill was engaged AT ALL, regardless of order. A positive + # criterion (skill_name == expected_skill) passes iff the expected skill + # was engaged somewhere in the run — a wrong skill engaged first does not + # fail it (recall). A distractor/negative criterion fails on ANY + # engagement of its skill (precision). + triggered: bool = criterion.skill_name in _all_engaged_skill_names(turn_records) expected_yes: bool = criterion.expected_skill == criterion.skill_name score = 1.0 if triggered == expected_yes else 0.0 observed = _YES if triggered else _NO @@ -147,29 +162,53 @@ def live_verdict( criterion: SkillTriggeredCriterion, turn_records: list[TurnRecord], ) -> LiveVerdict: - """First-engagement policy: the FIRST observed skill engagement decides. - - Activation measures which skill the agent selects *first*, so every - stacked ``skill_triggered`` criterion is decided simultaneously by the - first command that engages any skill: - - - before any engagement -> ``"undecided"``; - - on the first command engaging some skill: ``observed = (skill_name in - engaged)``, ``expected = (expected_skill == skill_name)`` -> - ``"pass"`` iff they match, else ``"fail"``. - - This covers the "wrong skill loads" case (a positive wrong signal) and - negative rows (``expected_skill == ""`` -> any engagement of the target - fails that criterion). ``_check_impl`` applies the SAME first-engagement - policy on the full trajectory, so the live verdict and the authoritative - score agree by construction — whether or not the run stopped early. + """Any-engagement latch: decide the instant THIS skill is engaged. + + Mirrors ``_check_impl``'s any-engagement policy, latched monotonically + over the growing partial trajectory: + + - this skill not engaged yet -> ``"undecided"`` (the final outcome still + depends on the rest of the run — the expected skill may load later, or + a distractor may yet fire); + - this skill engaged -> ``expected_skill == skill_name`` decides: + ``"pass"`` for a positive criterion (the expected skill loaded), + ``"fail"`` for a distractor/negative one (a wrong skill loaded). + + Because engagement is monotonic (a skill, once engaged, stays engaged), a + latched verdict never flips, so it agrees with ``_check_impl`` on the + frozen trajectory by construction — whether or not the run stopped early. + A positive criterion can therefore only ever live-``pass`` and a + distractor/negative one only ever live-``fail``; their *absence* is never + decidable mid-run (see ``live_decidable_polarities``). This is the change + from first-engagement: a wrong skill engaged first no longer live-fails a + positive row — the run keeps going so the expected skill can still load. """ - engaged = _first_engaged_skill_names(turn_records) - if not engaged: + if criterion.skill_name not in _all_engaged_skill_names(turn_records): return "undecided" + return "pass" if criterion.expected_skill == criterion.skill_name else "fail" + + @classmethod + def live_decidable_polarities(cls, criterion: SkillTriggeredCriterion) -> frozenset[str]: + """Per-instance narrowing under the any-engagement latch. + + Unlike the type-level capability (``live_stop_polarities`` = both), a + single instance decides exactly one polarity: + + - a **positive** criterion (``skill_name == expected_skill``) can only + live-``pass`` (the expected skill engaging is a decidable hit; its + absence is not knowable mid-run); + - a **distractor/negative** criterion (``skill_name != expected_skill``, + including the ``expected_skill == ""`` negatives) can only + live-``fail`` (a wrong skill engaging is a decidable miss; its absence + is not). + + ``validate_early_stop`` gates the requested ``stop_when`` on this set, so + arming a positive with ``fail`` / a distractor with ``pass`` — or either + with ``decided`` (which needs both) — is rejected at resolution rather + than silently degrading to a full run. + """ expected_yes = criterion.expected_skill == criterion.skill_name - observed_yes = criterion.skill_name in engaged - return "pass" if observed_yes == expected_yes else "fail" + return frozenset({"pass"}) if expected_yes else frozenset({"fail"}) def aggregate( self, diff --git a/src/coder_eval/orchestration/early_stop.py b/src/coder_eval/orchestration/early_stop.py index 7a4a61f3..a1d76fb6 100644 --- a/src/coder_eval/orchestration/early_stop.py +++ b/src/coder_eval/orchestration/early_stop.py @@ -296,8 +296,9 @@ def _evaluate(self, in_flight: CommandTelemetry | None = None) -> None: record = self._collector.build_turn_record() if in_flight is not None: # The in-flight call has no ToolEnd yet, so the collector (which - # reduces commands from ToolEnd) has not captured it. Append it and - # re-sort by sequence so the verdict sees it in first-engagement order. + # reduces commands from ToolEnd) has not captured it. Append it so its + # engagement is visible to the verdict; re-sort by sequence to keep the + # partial trajectory in emission order. record.commands = sorted([*record.commands, in_flight], key=lambda c: c.sequence_number) records = [record] # An in-flight call has not been counted by a ToolEnd yet, so report it as diff --git a/tests/test_early_stop.py b/tests/test_early_stop.py index e76fa0cc..e77b3983 100644 --- a/tests/test_early_stop.py +++ b/tests/test_early_stop.py @@ -282,10 +282,19 @@ def test_pass_when_expected_skill_engaged(self) -> None: rec = [_turn(_cmd("Skill", {"skill": "plugin:date-teller"}))] assert self.checker.live_verdict(crit, rec) == "pass" - def test_fail_when_wrong_skill_engaged(self) -> None: - # Positive row expecting date-teller, but a different skill loads first. + def test_positive_undecided_when_only_wrong_skill_engaged(self) -> None: + # A positive row expecting date-teller, but a different skill loads. Under + # any-engagement the positive criterion does NOT fail — date-teller may + # still load later, so the verdict stays undecided (the run keeps going). crit = _skill_crit("date-teller", "date-teller") rec = [_turn(_cmd("Skill", {"skill": "other-skill"}))] + assert self.checker.live_verdict(crit, rec) == "undecided" + + def test_distractor_fails_when_its_skill_engaged(self) -> None: + # A distractor criterion (skill_name != expected_skill): engaging its + # (wrong) skill is a decidable precision miss -> fail. + crit = _skill_crit("weather-teller", "date-teller") + rec = [_turn(_cmd("Skill", {"skill": "weather-teller"}))] assert self.checker.live_verdict(crit, rec) == "fail" def test_negative_row_target_engaged_is_fail(self) -> None: @@ -294,20 +303,33 @@ def test_negative_row_target_engaged_is_fail(self) -> None: rec = [_turn(_cmd("Skill", {"skill": "date-teller"}))] assert self.checker.live_verdict(crit, rec) == "fail" - def test_negative_row_other_engaged_is_pass(self) -> None: + def test_negative_undecided_when_other_engaged(self) -> None: + # A negative criterion cannot live-pass: the absence of its skill is not + # knowable mid-run, so an unrelated engagement leaves it undecided. crit = _skill_crit("date-teller", "") rec = [_turn(_cmd("Skill", {"skill": "unrelated"}))] - assert self.checker.live_verdict(crit, rec) == "pass" + assert self.checker.live_verdict(crit, rec) == "undecided" - def test_first_engagement_decides(self) -> None: - # The wrong skill engages first, the expected one later — first wins. + def test_expected_skill_engaged_after_wrong_is_pass(self) -> None: + # Item 1: the wrong skill engages first, the expected one later — the + # positive criterion passes (any-engagement, order-independent). crit = _skill_crit("date-teller", "date-teller") rec = [_turn(_cmd("Skill", {"skill": "wrong"}), _cmd("Skill", {"skill": "date-teller"}))] - assert self.checker.live_verdict(crit, rec) == "fail" + assert self.checker.live_verdict(crit, rec) == "pass" def test_polarities_declared(self) -> None: assert SkillTriggeredChecker.live_stop_polarities == frozenset({"pass", "fail"}) + def test_decidable_narrows_per_instance(self) -> None: + # A positive instance decides only pass; a distractor/negative only fail. + assert SkillTriggeredChecker.live_decidable_polarities(_skill_crit("date-teller", "date-teller")) == frozenset( + {"pass"} + ) + assert SkillTriggeredChecker.live_decidable_polarities( + _skill_crit("weather-teller", "date-teller") + ) == frozenset({"fail"}) + assert SkillTriggeredChecker.live_decidable_polarities(_skill_crit("date-teller", "")) == frozenset({"fail"}) + # --------------------------------------------------------------------------- # # command_executed live verdict @@ -409,10 +431,18 @@ def test_unobservable_checker_is_undecided(self) -> None: assert checker.live_verdict(crit, [_turn()]) == "undecided" def test_base_decidable_defaults_to_class_polarities(self) -> None: - # The base hook returns the ClassVar verbatim: skill_triggered does not - # narrow per-instance, so its decidable set equals its class capability. - crit = _skill_crit("s", "s") - assert SkillTriggeredChecker.live_decidable_polarities(crit) == SkillTriggeredChecker.live_stop_polarities + # The base hook returns the ClassVar verbatim for a criterion that does + # NOT override it: file_exists (unobservable) reports its empty capability. + init_criteria(validate=False) + checker_cls = type(CriterionRegistry.get_checker("file_exists")()) + crit = FileExistsCriterion(type="file_exists", path="x.txt", description="x") + assert checker_cls.live_decidable_polarities(crit) == checker_cls.live_stop_polarities == frozenset() + + def test_skill_triggered_decidable_is_subset_of_class_polarities(self) -> None: + # skill_triggered DOES narrow per-instance; each instance set stays a + # subset of the class capability. + for crit in (_skill_crit("s", "s"), _skill_crit("s", "other"), _skill_crit("s", "")): + assert SkillTriggeredChecker.live_decidable_polarities(crit) <= SkillTriggeredChecker.live_stop_polarities # --------------------------------------------------------------------------- # @@ -432,9 +462,28 @@ def test_unarmed_with_stop_when_is_inert(self) -> None: validate_early_stop(task) # no raise def test_armed_happy_path_accepts(self) -> None: - task = _task(criteria=[_skill_crit("s", "s", stop_when="decided")], stop_early=True) + # A positive skill_triggered decides only "pass", so arm it with pass. + task = _task(criteria=[_skill_crit("s", "s", stop_when="pass")], stop_early=True) + validate_early_stop(task) # no raise + + def test_armed_distractor_fail_accepts(self) -> None: + # A distractor (skill_name != expected_skill) decides only "fail". + task = _task(criteria=[_skill_crit("wrong", "s", stop_when="fail")], stop_early=True) validate_early_stop(task) # no raise + def test_skill_triggered_positive_fail_arm_rejected(self) -> None: + # A positive criterion can never live-fail; arming it with fail is a dead arm. + task = _task(criteria=[_skill_crit("s", "s", stop_when="fail")], stop_early=True) + with pytest.raises(EarlyStopConfigError, match="cannot decide polarity"): + validate_early_stop(task) + + def test_skill_triggered_decided_arm_rejected(self) -> None: + # A single skill_triggered instance decides only one polarity, so + # stop_when=decided (which needs both) can never be honored. + task = _task(criteria=[_skill_crit("s", "s", stop_when="decided")], stop_early=True) + with pytest.raises(EarlyStopConfigError, match="cannot decide polarity"): + validate_early_stop(task) + def test_armed_command_executed_accepts(self) -> None: # A decidable fail arm: must-NOT-run (max_count set) can live-fail. task = _task(criteria=[_cmd_crit(stop_when="fail", min_count=0, max_count=0)], stop_early=True) @@ -512,10 +561,12 @@ def test_raise_order_simulation_before_agent(self) -> None: validate_early_stop(task) def test_stacked_activation_criteria_accept(self) -> None: - # Multiple armed skill_triggered criteria (the activation pattern). + # The activation pattern under the any-engagement latch: the positive (GT) + # criterion arms pass, a distractor arms fail. `decided` is invalid for + # either because a single instance decides only one polarity. crits = [ - _skill_crit("skill-a", "skill-a", stop_when="decided"), - _skill_crit("skill-b", "skill-a", stop_when="decided"), + _skill_crit("skill-a", "skill-a", stop_when="pass"), # positive -> pass + _skill_crit("skill-b", "skill-a", stop_when="fail"), # distractor -> fail ] task = _task(criteria=crits, stop_early=True) validate_early_stop(task) # no raise @@ -540,7 +591,7 @@ def test_stacked_activation_criteria_accept(self) -> None: description: date-teller activation skill_name: date-teller expected_skill: date-teller - stop_when: decided + stop_when: pass """ @@ -849,7 +900,7 @@ def test_evaluation_result_roundtrip_with_early_stop(self) -> None: def test_armed_criteria_passed_gates_armed_only(self) -> None: # Armed skill passes; advisory file_exists fails. armed gate -> True. criteria = [ - _skill_crit("date-teller", "date-teller", stop_when="decided"), + _skill_crit("date-teller", "date-teller", stop_when="pass"), FileExistsCriterion(path="x", description="x must exist"), ] result = _result(criteria_results=[_crit_result("skill_triggered", 1.0), _crit_result("file_exists", 0.0)]) @@ -859,7 +910,7 @@ def test_armed_criteria_passed_gates_armed_only(self) -> None: def test_armed_criteria_passed_fails_when_armed_fails(self) -> None: criteria = [ - _skill_crit("date-teller", "date-teller", stop_when="decided"), + _skill_crit("date-teller", "date-teller", stop_when="pass"), FileExistsCriterion(path="x", description="x must exist"), ] result = _result(criteria_results=[_crit_result("skill_triggered", 0.0), _crit_result("file_exists", 1.0)]) @@ -893,7 +944,7 @@ class TestEarlyStopWatcher: def test_for_task_arms_only_stop_criteria(self) -> None: watcher = _watcher( [ - _skill_crit("date-teller", "date-teller", stop_when="decided"), + _skill_crit("date-teller", "date-teller", stop_when="pass"), FileExistsCriterion(path="x", description="x must exist"), ] ) @@ -901,50 +952,59 @@ def test_for_task_arms_only_stop_criteria(self) -> None: assert len(watcher._armed) == 1 def test_undecided_before_engagement_no_stop(self) -> None: - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")]) _feed(watcher, [_agent_start(), _turn_start()]) assert watcher.should_stop() is False assert watcher.info is None def test_pass_stop_fires_on_expected_skill(self) -> None: - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")]) _feed(watcher, _skill_events("date-teller")) assert watcher.should_stop() is True assert watcher.info is not None assert watcher.info.reason == EarlyStopReason.CRITERION_PASSED - def test_fail_stop_fires_on_wrong_skill(self) -> None: - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + def test_fail_stop_fires_on_distractor_skill(self) -> None: + # A distractor criterion (its skill != the expected skill) fail-stops the + # instant its skill is engaged — the per-skill precision signal. + watcher = _watcher([_skill_crit("weather-teller", "date-teller", stop_when="fail")]) _feed(watcher, _skill_events("weather-teller")) assert watcher.should_stop() is True assert watcher.info is not None assert watcher.info.reason == EarlyStopReason.CRITERION_FAILED - def test_pass_polarity_does_not_fire_on_fail(self) -> None: - # stop_when="pass": a wrong-skill (live-fail) engagement must NOT stop. + def test_wrong_skill_does_not_stop_positive_row(self) -> None: + # Item 1: a positive row (armed pass) engaging the WRONG skill must NOT + # stop — the run keeps going so the expected skill can still load later. watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")]) _feed(watcher, _skill_events("weather-teller")) assert watcher.should_stop() is False + assert watcher.info is None - def test_stacked_pass_stop(self) -> None: - # Two armed skill criteria, both expecting date-teller; engaging it passes both. + def test_stacked_pass_stop_requires_all(self) -> None: + # Pass-stop needs EVERY armed criterion to live-pass. Two positives for + # different skills: engaging only the first does not stop; engaging the + # second (both now passed) fires the pass-stop. watcher = _watcher( [ - _skill_crit("date-teller", "date-teller", stop_when="decided"), - _skill_crit("weather-teller", "", stop_when="decided"), + _skill_crit("date-teller", "date-teller", stop_when="pass"), + _skill_crit("weather-teller", "weather-teller", stop_when="pass"), ] ) _feed(watcher, _skill_events("date-teller")) + assert watcher.should_stop() is False # only one of two has passed + _feed(watcher, [_tool_end(_skill_cmd("weather-teller", tool_id="w"))]) assert watcher.should_stop() is True assert watcher.info is not None assert watcher.info.reason == EarlyStopReason.CRITERION_PASSED def test_stacked_wrong_skill_fail_stop(self) -> None: - # Engaging weather-teller: date-teller row -> fail; the fail-stop fires first. + # A positive (armed pass) + a distractor (armed fail). Engaging the + # distractor's skill fires the fail-stop while the positive stays undecided. watcher = _watcher( [ - _skill_crit("date-teller", "date-teller", stop_when="decided"), - _skill_crit("weather-teller", "date-teller", stop_when="decided"), + _skill_crit("date-teller", "date-teller", stop_when="pass"), + _skill_crit("weather-teller", "date-teller", stop_when="fail"), ] ) _feed(watcher, _skill_events("weather-teller")) @@ -952,26 +1012,26 @@ def test_stacked_wrong_skill_fail_stop(self) -> None: assert watcher.info.reason == EarlyStopReason.CRITERION_FAILED def test_records_turn_and_tool_index(self) -> None: - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")]) _feed(watcher, _skill_events("date-teller")) assert watcher.info is not None assert watcher.info.sdk_turn_index == 1 assert watcher.info.tool_call_index == 1 def test_turns_remaining_from_max_turns(self) -> None: - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")], max_turns=15) + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")], max_turns=15) _feed(watcher, _skill_events("date-teller")) assert watcher.info is not None assert watcher.info.turns_remaining_at_stop == 14 # 15 - sdk_turn_index(1) def test_turns_remaining_none_when_max_turns_unset(self) -> None: - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")], max_turns=None) + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")], max_turns=None) _feed(watcher, _skill_events("date-teller")) assert watcher.info is not None assert watcher.info.turns_remaining_at_stop is None def test_fail_open_on_raising_verdict(self) -> None: - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")]) with patch.object(SkillTriggeredChecker, "live_verdict", side_effect=RuntimeError("boom")): _feed(watcher, _skill_events("date-teller")) # Fail-open: disarmed, no false stop, degrades to a full run. @@ -984,7 +1044,7 @@ def test_unresolved_tool_end_does_not_latch(self) -> None: # loop ends and the terminal status is chosen. Such an orphan Skill # engagement must NOT trip a stop, else a naturally-completed (or # timed-out / crashed) run gets recorded as early-stopped. - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")]) _feed(watcher, [_agent_start(), _turn_start(), _unresolved_skill_end("date-teller")]) assert watcher.should_stop() is False assert watcher.info is None @@ -993,7 +1053,7 @@ def test_unresolved_tool_end_does_not_latch(self) -> None: def test_resolved_after_unresolved_still_decides(self) -> None: # An UNRESOLVED end is dropped, but a later RESOLVED engagement still fires # the stop (dropping orphans never suppresses a real, observed stop). - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")]) _feed(watcher, [_agent_start(), _turn_start(), _unresolved_skill_end("date-teller")]) assert watcher.info is None _feed(watcher, [_tool_end(_skill_cmd("date-teller", tool_id="sk-real"))]) @@ -1002,7 +1062,7 @@ def test_resolved_after_unresolved_still_decides(self) -> None: assert watcher.info.reason == EarlyStopReason.CRITERION_PASSED def test_decision_latched_after_fire(self) -> None: - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")]) _feed(watcher, _skill_events("date-teller")) fired = watcher.info # A subsequent (wrong-skill) engagement must not overwrite the latched decision. @@ -1015,7 +1075,7 @@ def test_tool_call_fires_before_result(self) -> None: # The decision latches on the tool CALL (ToolStartEvent): a Skill call # whose result never arrives (a cut-short turn would strip it) still stops. # No ToolEndEvent is ever fed. - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")]) _feed(watcher, [_agent_start(), _turn_start(), _skill_start("date-teller")]) assert watcher.should_stop() is True assert watcher.info is not None @@ -1023,17 +1083,36 @@ def test_tool_call_fires_before_result(self) -> None: # The in-flight call reports as the 1st tool call even without a ToolEnd. assert watcher.info.tool_call_index == 1 - def test_tool_call_wrong_skill_fail_fires(self) -> None: - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + def test_tool_call_distractor_fail_fires(self) -> None: + # A distractor (armed fail) fail-stops on the tool CALL that engages its + # skill, before any result arrives. + watcher = _watcher([_skill_crit("weather-teller", "date-teller", stop_when="fail")]) _feed(watcher, [_agent_start(), _turn_start(), _skill_start("weather-teller")]) assert watcher.info is not None assert watcher.info.reason == EarlyStopReason.CRITERION_FAILED + def test_tool_call_latches_on_file_read_engagement(self) -> None: + # Off-Claude agents (antigravity/codex) engage a skill by READING its files + # (skills//...), not via a Skill tool call. The watcher must latch on + # that Read ToolStart — the file-path parameter carries the signal on the + # call itself, so early-stop fires off-Claude just as it does for Claude. + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")]) + read = CommandTelemetry( + tool_name="Read", + tool_id="r1", + timestamp=_TS, + parameters={"file_path": "/repo/skills/date-teller/SKILL.md"}, + ) + _feed(watcher, [_agent_start(), _turn_start(), ToolStartEvent(task_id="t", tool=read)]) + assert watcher.should_stop() is True + assert watcher.info is not None + assert watcher.info.reason == EarlyStopReason.CRITERION_PASSED + def test_tool_call_latches_before_unresolved_end(self) -> None: # The call fires the stop in-loop; a later finalize() UNRESOLVED end for # the SAME call is short-circuited (decision already latched) — no relabel, # no double count. - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")]) _feed(watcher, [_agent_start(), _turn_start(), _skill_start("date-teller", tool_id="sk-1")]) fired = watcher.info _feed(watcher, [_unresolved_skill_end("date-teller", tool_id="sk-1")]) @@ -1044,7 +1123,7 @@ def test_tool_call_latches_before_unresolved_end(self) -> None: def test_tool_call_index_counts_prior_resolved_calls(self) -> None: # A prior resolved, non-deciding tool is counted at its ToolEnd; the # deciding in-flight call is then reported as the next (2nd) call. - watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="decided")]) + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")]) prior = _cmd("Bash", {"command": "ls"}) # not a skill engagement _feed(watcher, [_agent_start(), _turn_start(), _tool_end(prior)]) assert watcher.info is None @@ -1142,13 +1221,20 @@ async def _run_wiring( class TestOrchestratorEarlyStopWiring: _SKILL = "date-teller" - def _criteria(self, *, expected: str = "date-teller", stop_when: str | None = "decided") -> list[Any]: - # Armed skill_triggered + advisory file_exists (deliberately failing). + def _criteria(self, *, expected: str = "date-teller", stop_when: str | None = "pass") -> list[Any]: + # Armed positive skill_triggered + advisory file_exists (deliberately failing). return [ _skill_crit(self._SKILL, expected, stop_when=stop_when), FileExistsCriterion(path="artifact.txt", description="artifact must exist"), ] + def _distractor_criteria(self) -> list[Any]: + # A distractor (armed fail) + advisory file_exists, for the fail-stop path. + return [ + _skill_crit("weather-teller", self._SKILL, stop_when="fail"), + FileExistsCriterion(path="artifact.txt", description="artifact must exist"), + ] + async def test_default_off_full_gate_no_early_stop(self, tmp_path) -> None: # Unarmed: no watcher, all criteria gate, advisory 0.0 drags to FAILURE. result, agent = await _run_wiring( @@ -1176,8 +1262,9 @@ async def test_pass_stop_cuts_the_stream(self, tmp_path) -> None: assert result.early_stop.reason == EarlyStopReason.CRITERION_PASSED async def test_fail_stop_wiring(self, tmp_path) -> None: + # A distractor (armed fail) fires the fail-stop when its skill is engaged. result, _agent = await _run_wiring( - criteria=self._criteria(), + criteria=self._distractor_criteria(), events=_skill_events("weather-teller"), scores=[0.0, 0.0], stop_early=True, diff --git a/tests/test_skill_triggered.py b/tests/test_skill_triggered.py index d2332dd0..b9d0100b 100644 --- a/tests/test_skill_triggered.py +++ b/tests/test_skill_triggered.py @@ -98,11 +98,11 @@ def test_no_turn_records_returns_base_result_with_error(self) -> None: assert result.error is not None -class TestSkillTriggeredFirstEngagement: - """First-engagement scoring: only the FIRST skill the agent engages is scored. - - A second skill invoked alongside or after the first is not counted as a - competing activation, so it is not a false positive against its own criterion. +class TestSkillTriggeredAnyEngagement: + """Any-engagement scoring: a skill counts if it was engaged *at all*, in any + order. The expected skill passes its criterion (recall) even when a wrong + skill was touched first; an off-target skill still fails its own criterion + (precision). """ def _admin_platform(self) -> list[CommandTelemetry]: @@ -112,26 +112,50 @@ def _admin_platform(self) -> list[CommandTelemetry]: _cmd("Skill", {"skill": "uipath-platform"}, tool_id="s2"), ] - def test_first_skill_is_true_positive(self) -> None: - # GT=uipath-admin; admin engaged first -> observed=yes, expected=yes. + def test_expected_skill_engaged_first_is_true_positive(self) -> None: + # GT=uipath-admin; admin engaged (first) -> observed=yes, expected=yes. result = _check(expected_skill="uipath-admin", skill_name="uipath-admin", commands=self._admin_platform()) assert result.observed_label == "yes" and result.score == 1.0 - def test_second_skill_not_false_positive(self) -> None: - # Same run scored for the uipath-platform criterion: platform is NOT the - # first engagement -> observed=no; expected=no (GT is admin) -> pass, no FP. + def test_off_target_skill_engaged_is_false_positive(self) -> None: + # Same run scored for the uipath-platform criterion: platform WAS engaged + # (second), so on an admin row it is a precision miss -> observed=yes, + # expected=no -> score 0.0. This is the per-skill precision signal. result = _check(expected_skill="uipath-admin", skill_name="uipath-platform", commands=self._admin_platform()) - assert result.observed_label == "no" and result.score == 1.0 + assert result.observed_label == "yes" and result.score == 0.0 - def test_wrong_skill_first_still_penalized(self) -> None: - # If the WRONG skill engages first, the GT criterion correctly fails — - # first-engagement does not mask a genuine misfire. + def test_expected_skill_engaged_after_wrong_still_passes(self) -> None: + # Item 1: the WRONG skill engages first, the expected one later. The GT + # criterion must still PASS — an earlier wrong touch (comparison, not + # commitment) does not fail the row. commands = [ _cmd("Skill", {"skill": "uipath-platform"}, tool_id="s1"), _cmd("Skill", {"skill": "uipath-admin"}, tool_id="s2"), ] result = _check(expected_skill="uipath-admin", skill_name="uipath-admin", commands=commands) - assert result.observed_label == "no" and result.score == 0.0 + assert result.observed_label == "yes" and result.score == 1.0 + + def test_negative_row_fails_on_any_engagement(self) -> None: + # Negative row (expected_skill == ""): engaging the target skill at all — + # even after an unrelated one — is a false positive. + commands = [ + _cmd("Skill", {"skill": "uipath-platform"}, tool_id="s1"), + _cmd("Skill", {"skill": "uipath-admin"}, tool_id="s2"), + ] + result = _check(expected_skill="", skill_name="uipath-admin", commands=commands) + assert result.observed_label == "yes" and result.score == 0.0 + + def test_stacked_recall_and_precision_on_one_trajectory(self) -> None: + # How the stacked criteria score a single positive row (GT=uipath-admin) + # on which the agent engaged BOTH the expected skill and an off-target one. + # The GT criterion credits recall (pass); the off-target criterion records + # a precision miss (fail). No precision hole: an extra engagement is never + # silently absorbed — it lands on its own skill's confusion cell. + commands = self._admin_platform() + recall = _check(expected_skill="uipath-admin", skill_name="uipath-admin", commands=commands) + precision = _check(expected_skill="uipath-admin", skill_name="uipath-platform", commands=commands) + assert recall.observed_label == "yes" and recall.score == 1.0 # recall: GT engaged + assert precision.observed_label == "yes" and precision.score == 0.0 # precision: off-target engaged class TestSkillTriggeredCodex: From 603e23c948e48c7ca0f65b44ca210b6834a3d5af Mon Sep 17 00:00:00 2001 From: mohsen-uipath <> Date: Thu, 23 Jul 2026 10:30:27 -0700 Subject: [PATCH 3/3] test(early-stop): cover second-review items (two-AgentStart, golden corpus, parity) Add the still-applicable coverage from the second review; no scoring-logic change (any-engagement was already restored by the prior commit): - Golden-corpus regression test pinning observed_label AND score for the canonical skill_triggered trajectories (single-target, target-then-competitor recall/precision, wrong-then-target, negative false-positive, true-negative, file-read), so a future engagement-policy change fails loudly instead of silently shifting the activation suite's P/R/F1. - Multi-turn + file-read parity tests: any-engagement holds across TurnRecords and for the file-read signal (Read of skills//...), not only the Claude Skill call, so the agent-agnostic parity is asserted for the new branch. - Two-AgentStart watcher test: a retry's second AgentStart does not reset the wall-clock origin (the documented no-op branch in on_event). Also clarifies the EarlyStopInfo.tool_call_index field doc: on a call-latched stop it is the deciding in-flight call index (completed_tool_ends + 1), read as "which call decided", not a count of fully-completed tool calls. --- src/coder_eval/models/results.py | 7 +- tests/test_early_stop.py | 17 ++++ tests/test_skill_triggered.py | 149 +++++++++++++++++++++++++++++++ 3 files changed, 172 insertions(+), 1 deletion(-) diff --git a/src/coder_eval/models/results.py b/src/coder_eval/models/results.py index 05dbbb1d..433aa8c6 100644 --- a/src/coder_eval/models/results.py +++ b/src/coder_eval/models/results.py @@ -420,7 +420,12 @@ class EarlyStopInfo(BaseModel): description="SDK inner-turn count at the stop (watcher counts TurnStartEvents). NOT the " + "orchestrator iteration, which is always 1 in single-shot." ) - tool_call_index: int = Field(description="1-based index of the tool call that decided the stop.") + tool_call_index: int = Field( + description="1-based index of the tool call that decided the stop. NOTE: because the " + + "stop latches on the tool CALL (ToolStartEvent), this is the index of the deciding " + + "call INCLUDING that in-flight call — i.e. completed_tool_ends + 1 for a call-latched " + + "stop. Read it as 'which call decided', not as a count of fully-completed tool calls." + ) elapsed_seconds: float = Field(description="Wall-clock seconds from the first agent-start event to the stop.") turns_remaining_at_stop: int | None = Field( default=None, diff --git a/tests/test_early_stop.py b/tests/test_early_stop.py index e77b3983..80998978 100644 --- a/tests/test_early_stop.py +++ b/tests/test_early_stop.py @@ -1131,6 +1131,23 @@ def test_tool_call_index_counts_prior_resolved_calls(self) -> None: assert watcher.info is not None assert watcher.info.tool_call_index == 2 + def test_second_agent_start_does_not_reset_origin(self) -> None: + # The wall-clock origin is stamped at the FIRST AgentStartEvent only; a + # retry's second AgentStart must NOT reset it (the documented no-op branch + # in on_event). Exercised deterministically via _started_monotonic rather + # than the time-based elapsed_seconds field. + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_when="pass")]) + _feed(watcher, [_agent_start()]) + origin = watcher._started_monotonic + assert origin is not None + # A second AgentStart (as on a retry) must leave the origin untouched. + _feed(watcher, [_agent_start(), _turn_start()]) + assert watcher._started_monotonic == origin + # The stop that follows anchors elapsed_seconds to that first origin. + _feed(watcher, [_skill_start("date-teller")]) + assert watcher.info is not None + assert watcher.info.elapsed_seconds >= 0.0 + # --------------------------------------------------------------------------- # # Phase 3: Orchestrator wiring diff --git a/tests/test_skill_triggered.py b/tests/test_skill_triggered.py index b9d0100b..a856cde5 100644 --- a/tests/test_skill_triggered.py +++ b/tests/test_skill_triggered.py @@ -158,6 +158,155 @@ def test_stacked_recall_and_precision_on_one_trajectory(self) -> None: assert precision.observed_label == "yes" and precision.score == 0.0 # precision: off-target engaged +def _check_multi( + *, expected_skill: str, skill_name: str, turns: list[list[CommandTelemetry]] +) -> ClassificationCriterionResult: + criterion = SkillTriggeredCriterion( + description="did agent invoke a skill?", + expected_skill=expected_skill, + skill_name=skill_name, + ) + checker = SkillTriggeredChecker() + turn_records = [_turn(cmds) for cmds in turns] + result = checker.check(criterion, sandbox=None, turn_records=turn_records) # type: ignore[arg-type] + assert isinstance(result, ClassificationCriterionResult) + return result + + +class TestSkillTriggeredGoldenCorpus: + """Regression lock: pins observed_label AND score for the canonical + multi-skill trajectories through ``_check_impl``. Any future change to the + engagement policy (any- vs first-engagement) breaks these, forcing an + explicit acknowledgement of the methodology break — and a re-score/backfill + of historical activation P/R/F1 — before merge. + """ + + @pytest.mark.parametrize( + ("case", "expected_skill", "skill_name", "commands", "exp_observed", "exp_score"), + [ + ( + "single-target-recall", + "uipath-admin", + "uipath-admin", + [_cmd("Skill", {"skill": "uipath-admin"}, tool_id="s1")], + "yes", + 1.0, + ), + ( + "target-first-then-competitor-recall", + "uipath-admin", + "uipath-admin", + [ + _cmd("Skill", {"skill": "uipath-admin"}, tool_id="s1"), + _cmd("Skill", {"skill": "uipath-platform"}, tool_id="s2"), + ], + "yes", + 1.0, + ), + ( + "target-first-then-competitor-precision", + "uipath-admin", + "uipath-platform", + [ + _cmd("Skill", {"skill": "uipath-admin"}, tool_id="s1"), + _cmd("Skill", {"skill": "uipath-platform"}, tool_id="s2"), + ], + "yes", + 0.0, + ), + ( + "wrong-first-then-target-recall", + "uipath-admin", + "uipath-admin", + [ + _cmd("Skill", {"skill": "uipath-platform"}, tool_id="s1"), + _cmd("Skill", {"skill": "uipath-admin"}, tool_id="s2"), + ], + "yes", + 1.0, + ), + ( + "negative-target-engaged-false-positive", + "", + "uipath-admin", + [_cmd("Skill", {"skill": "uipath-admin"}, tool_id="s1")], + "yes", + 0.0, + ), + ( + "negative-no-engagement-true-negative", + "", + "uipath-admin", + [_cmd("Read", {"file_path": "notes.txt"})], + "no", + 1.0, + ), + ( + "file-read-target-recall", + "uipath-admin", + "uipath-admin", + [_cmd("Read", {"file_path": "skills/uipath-admin/SKILL.md"})], + "yes", + 1.0, + ), + ], + ) + def test_golden_corpus_scores_are_pinned( + self, + case: str, + expected_skill: str, + skill_name: str, + commands: list[CommandTelemetry], + exp_observed: str, + exp_score: float, + ) -> None: + result = _check(expected_skill=expected_skill, skill_name=skill_name, commands=commands) + assert result.observed_label == exp_observed, case + assert result.score == exp_score, case + + +class TestSkillTriggeredMultiTurnParity: + """Any-engagement holds across TurnRecords and for the file-read signal, so + the agent-agnostic parity CLAUDE.md stresses is asserted for the new branch. + """ + + def test_expected_skill_in_later_turn_still_recalls(self) -> None: + # Distractor engaged in turn 1, expected skill in turn 2. Recall must + # credit the GT criterion regardless of which turn the engagement lives in. + result = _check_multi( + expected_skill="uipath-admin", + skill_name="uipath-admin", + turns=[ + [_cmd("Skill", {"skill": "uipath-platform"}, tool_id="s1")], + [_cmd("Skill", {"skill": "uipath-admin"}, tool_id="s2")], + ], + ) + assert result.observed_label == "yes" and result.score == 1.0 + + def test_off_target_in_earlier_turn_is_precision_miss(self) -> None: + # The competitor engaged in an EARLIER turn than the GT skill still lands + # as a precision miss on its own criterion (no turn-order dependence). + turns = [ + [_cmd("Skill", {"skill": "uipath-platform"}, tool_id="s1")], + [_cmd("Skill", {"skill": "uipath-admin"}, tool_id="s2")], + ] + precision = _check_multi(expected_skill="uipath-admin", skill_name="uipath-platform", turns=turns) + assert precision.observed_label == "yes" and precision.score == 0.0 + + def test_file_read_parity_across_turns(self) -> None: + # Off-Claude parity: the agent READS skill files across turns (platform's + # in turn 1, admin's in turn 2). Recall/precision score identically to the + # Skill-tool path, order- and turn-independent. + turns = [ + [_cmd("Read", {"file_path": "skills/uipath-platform/SKILL.md"})], + [_cmd("Read", {"file_path": "skills/uipath-admin/reference.md"})], + ] + recall = _check_multi(expected_skill="uipath-admin", skill_name="uipath-admin", turns=turns) + precision = _check_multi(expected_skill="uipath-admin", skill_name="uipath-platform", turns=turns) + assert recall.observed_label == "yes" and recall.score == 1.0 + assert precision.observed_label == "yes" and precision.score == 0.0 + + class TestSkillTriggeredCodex: """Codex has no ``Skill`` tool — it engages a skill by reading its files via shell.