fix(early-stop): decide skill activation on the tool call, not its result

[mohsen-uipath]

Summary

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.

Changes

• orchestration/early_stop.py — decide on the tool call. The watcher now evaluates armed criteria on ToolStartEvent (the call) 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.

• criteria/skill_triggered.py — first-engagement scoring. The final check now scores 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.py — quiet the rate-limit warning. 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.

Tests

Adds coverage for:

• the tool-call decision (call fires before/without a result, latches before a later UNRESOLVED end, tool_call_index accounting);
• the orchestrator cut on a resultless call;
• first-engagement scoring, including the parallel-call case;
• the rate-limit event skip.

All existing early-stop / skill-triggered tests pass unchanged.

Verification

• ruff format + ruff check clean
• pyright — 0 errors
• full test suite — all green
• custom architectural lint (CE001–CE025) — all green

[github-actions]

Claude finished @mohsen-uipath's task in 1m 12s —— View job

---

Code Review in Progress

Todo List:

• Read code review guidelines from .github/code_review.md
• Read project conventions from CLAUDE.md
• Analyze PR diff and understand changes
• Read full files for context (not just diffs)
• Review implementation correctness
• Check cross-file consistency
• Perform "what's missing" analysis
• Design-level scrutiny
• Format and submit final review

[bai-uipath]

Approve the watcher fix in principle, but change the scoring policy before this lands — and note that on the 2026-07-22 run (antigravity/gemini), #43 alone moves almost nothing. Deciding on the tool call instead of its stripped result is the right root-cause fix.

Change requests

1. Decide on the expected-skill hit, not first-engagement. criteria/skill_triggered.py latches on whichever skill is engaged first and fails the row if it's the wrong one. Change it to pass-latch (and early-stop) when the expected skill is engaged; a wrong skill engaged before it should not fail the row — keep going and pass if the expected skill is eventually engaged. On this run that recovers uipath-agents-007 (platform→agents) and uipath-maestro-case-038 (solution→case), which hit the correct skill but not first. On the file-read harnesses reading a SKILL.md is comparison, not commitment, so "first file touched = verdict" penalizes exploration the metric shouldn't care about.
2. Keep the negative side honest. With "pass if the expected skill is ever engaged," negative rows (expected_skill == "") must still fail on any engagement, and a positive row that also fires an unrelated skill still needs a precision signal — spell out how the stacked criteria score so this doesn't open a precision hole.
3. Add the uipath-connector-builder activation prompts (skills repo). It's absent from the suite entirely — no uipath-connector-builder.jsonl and no criterion in tests/tasks/activation/activation.yaml (24 datasets, 23 criteria), though the skill exists — which is why it reads 0%. Add the dataset + stacked criterion; belongs in skills (#2028 or new), not #43.

Footguns

• Not armed on main. early_stop.py:204 arms only criteria with stop_when != null, gated on run_limits.stop_early — the activation config on main has neither (arming is in the still-open skills #2028). #43's watcher path is dormant until #2028 merges; land them together.
• max_turns is dead code for antigravity. agents/antigravity_agent.py:411 accepts it and never uses it — gemini ran 27–52 turns under max_turns: 1. So #2028's max_turns: 3 is also a no-op there; antigravity relies entirely on the armed early-stop firing, and a row whose skill never triggers runs to the 120s wall.
• Confirm the watcher fires on a file-read, not just a Skill call — antigravity/codex engage via Read/Grep of skills/<name>/…. If live_verdict doesn't latch on that ToolStartEvent, early-stop never triggers off-Claude.
• Timeouts are out of scope and stay broken. 149/480 (31%) are turn-0 120s stalls with no tool call to latch; neither #43 nor #2028 touches them — needs higher task_timeout / lower parallelism, tracked separately.

[uipreliga]

Review: coder_eval — pr:43

Scope: pr:43 · branch fix/early-stop-tool-call · d1cb685 · 2026-07-23T00:44Z · workflow variant

Change class: complex — changes early-stop control flow (decide on the tool call, not its result) and the skill_triggered scoring policy (first-engagement)

coder_eval is exceptionally healthy (9.9/10, seven of eight axes at 10 with zero critical/high defects across code quality, type safety, security, architecture, and error handling), and the only real risk is confined to the Evaluation Harness axis where the skill_triggered any-engagement → first-engagement rescoring silently step-changes the nightly activation suite's P/R/F1 for identical agent traces — a methodology break that must be annotated/backfilled rather than read as an agent regression, but nothing that undermines the framework's fundamentals.

Summary

Axis	Score	🔴	🟠	🟡	🔵	Top Issue
1. Code Quality & Style	10 / 10	0	0	0	0	—
2. Type Safety	10 / 10	0	0	0	0	—
3. Test Health	9.9 / 10	0	0	0	1	Documented retry-idempotent wall-clock stamp branch is untested
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	8.9 / 10	0	1	0	1	skill_triggered scoring semantics change (any- -> first-engagement) shifts activation-suite metrics for identical traces

Overall Score: 9.9 / 10 · Weakest Axis: Evaluation Harness Quality at 8.9 / 10
Totals: 🔴 0 · 🟠 1 · 🟡 0 · 🔵 2 across 8 axes.

Blockers

1. [Axis 8] skill_triggered scoring semantics change (any- -> first-engagement) shifts activation-suite metrics for identical traces (src/coder_eval/criteria/skill_triggered.py:129) — _check_impl changed from any-engagement — any(_engaged_skill(cmd, criterion.skill_name) for turn in turn_records for cmd in turn.commands) — to first-engagement: line 129 is now triggered: bool = criterion.skill_name in _first_engaged_skill_names(turn_records), which only considers the FIRST command that engages ANY skill. For an identical, previously-recorded agent trajectory this changes the persisted per-row observed_label (and thus score) whenever the agent engages more than one skill: (a) target-skill-first-then-competitor now scores the competitor's row observed='no' instead of 'yes' (removes a false positive), and (b) wrong-skill-first-then-target now scores the target row observed='no' instead of 'yes' (turns a prior true-positive into a false-negative). Per SkillTriggeredCriterion's model docstring (models/criteria.py:582) the activation suite stacks one criterion per skill and rolls them up into per-skill accuracy/recall/F1/confusion with a suite_thresholds: {recall.yes: ...} gate, so this shifts the suite's headline metrics and can flip the suite gate for the SAME traces. This is the Axis-8 scoring-correctness class (score changes for identical agent output). The new policy is arguably more correct (and it makes live_verdict and _check_impl agree by construction, fixing a prior inconsistency), so the recommendation is not to revert but to state the blast radius the PR omits: the nightly activation suite's metrics step-change at this commit and the dashboard/trend consumer now lives in the separate coder-eval-uipath / eval-runner repo, so any historical activation P/R/F1 must be re-scored/backfilled or annotated as a methodology break rather than read as an agent regression.

Non-blocking, but please consider before merge

None.

Nits

1. [Axis 3] Documented retry-idempotent wall-clock stamp branch is untested (tests/test_early_stop.py:892) — early_stop.py:261 if self._started_monotonic is None: is a deliberate, documented behavior (on_event docstring: "stamps the wall-clock origin at the FIRST AgentStartEvent only (a retry's second AgentStart does not reset it)"), but no test in TestEarlyStopWatcher feeds two AgentStartEvents, so branch 261->277 (the second-AgentStart no-op) is never exercised. Add a test that feeds [_agent_start(), _agent_start(), ...] and asserts elapsed_seconds is anchored to the first start (i.e. the second AgentStart does not reset the origin). Impact is limited to the elapsed_seconds telemetry field, not the score or final_status, hence Low.
2. [Axis 8] _first_engaged_skill_names relies on unenforced caller command ordering (src/coder_eval/criteria/skill_triggered.py:75) — _first_engaged_skill_names (lines 75-88) returns the engaged set of the FIRST command in iteration order and its docstring asserts 'Commands are scanned in sequence_number order (turn records preserve it)' (line 83), but the function itself never sorts — it trusts each turn.commands to already be in sequence_number order. It is correct today only because both call sites establish that order externally: EventCollector.build_turn_record sorts via _ordered_commands (collector.py:85-86) for the _check_impl path, and early_stop.py:298 re-sorts (record.commands = sorted([*record.commands, in_flight], key=lambda c: c.sequence_number)) for the in-flight path. A future caller passing an unsorted command list would silently pick the wrong 'first' skill and mis-score the row with no error. Make the function self-contained by sorting on sequence_number internally (cheap, idempotent given callers already sort) so first-engagement correctness does not depend on an unenforced caller invariant.

What's Missing

Daily/nightly:

• 🟠 skill_triggered.py switched activation scoring from any-engagement to first-engagement, so per-row observed_label/score change for identical, previously-recorded traces. The PR does not state the blast radius on the production nightly activation suite: its headline accuracy/recall/F1/confusion step-change at this commit, and the dashboard/trend consumer now lives in the separate coder-eval-uipath / eval-runner repo. The gap is the unstated methodology break — historical activation metrics must be re-scored/backfilled or annotated, not read as an agent regression. (trigger: src/coder_eval/criteria/skill_triggered.py) (restates: Axis 8: skill_triggered scoring semantics change (any- -> first-engagement) shifts activation-suite metrics for identical traces)

Tests:

• 🟡 The new first-engagement policy (_first_engaged_skill_names) is the shared scoring path for both agents, but TestSkillTriggeredFirstEngagement exercises ordering only via Claude Skill-tool calls. No test covers first-engagement ordering for the Codex file-read signal (a run that reads skill A's files first then a competing skill B's) nor a first engagement that lives in an earlier TurnRecord across multiple turns. Add a Codex/multi-turn ordering case so the agent-agnostic parity CLAUDE.md stresses is actually asserted for the new branch. (trigger: tests/test_skill_triggered.py)
• 🔵 _evaluate(in_flight=...) appends the in-flight command and re-sorts by sequence_number (early_stop.py:298) so first-engagement order is honored, but no test feeds an in-flight call whose sequence_number places it BEFORE an already-collected command (the only case where the sort actually reorders and changes which skill is 'first'). Existing tests append with equal/later sequence, so the load-bearing re-sort branch is never exercised. (trigger: tests/test_early_stop.py) _(restates: Axis 8: first_engaged_skill_names relies on unenforced caller command ordering)
• 🔵 The documented retry-idempotent wall-clock stamp (on_event: 'stamps the origin at the FIRST AgentStartEvent only') has no test in TestEarlyStopWatcher feeding two AgentStartEvents to assert the second does not reset _started_monotonic, so that no-op branch is unexercised. (trigger: tests/test_early_stop.py) (restates: Axis 3: Documented retry-idempotent wall-clock stamp branch is untested)

Downstream consumers:

• 🔵 EarlyStopInfo.tool_call_index now reports the in-flight (uncompleted) tool call as _tool_call_index + 1 when a stop latches on ToolStartEvent, rather than a count of completed ToolEnds. Per CLAUDE.md this field surfaces in report notes/badges and telemetry dims (and the cross-repo evalboard). The PR does not state whether any consumer that reads tool_call_index as 'completed tool calls at stop' needs adjusting for the now-possible off-by-one on a call-latched stop. (trigger: src/coder_eval/orchestration/early_stop.py)

Harness & Lint Improvements

Static checks (lint / type):

• [ce-lint] Add CE026 (next free CE number) no-first-of-commands-without-sequence-sort in tests/lint/rules/ce026_commands_ordering.py, wired into ALL_RULES in tests/lint/runner.py. Scope it to src/coder_eval/criteria/ and src/coder_eval/orchestration/early_stop.py and flag any order-sensitive 'first element' access of a *.commands sequence — .commands[0], next(iter(... .commands ...)), or a for-loop over .commands that returns/breaks on the first match — unless a sorted(..., key=lambda c: c.sequence_number) is applied to that sequence in the enclosing function (mirroring EventCollector._ordered_commands and early_stop.py's re-sort). This forbids the exact positional-coupling shape where 'first-engagement' correctness silently depends on an unenforced caller ordering invariant; a # noqa: CE026 covers a genuinely order-independent first-match loop. Prevents: Finding 3 (skill_triggered.py:75 _first_engaged_skill_names returns the engaged set of the first command in iteration order but never sorts, trusting turn.commands to already be sequence_number-ordered — a future caller passing unsorted commands would silently pick the wrong 'first' skill and mis-score the row).

Harness improvements (not statically reachable):

• Add a scoring golden-corpus regression test for skill_triggered: a fixture of hand-built multi-skill turn_records (target-first-then-competitor, wrong-first-then-target, single-target, and a negative expected_skill='' row) with the expected per-row observed_label AND score pinned, asserted through SkillTriggeredChecker._check_impl. Any future change to the engagement policy (any- vs first-engagement) then fails a test, forcing an explicit acknowledgement of the methodology break and a re-score/backfill (or annotation) of the historical activation P/R/F1/confusion metrics before merge. Why not static: The defect is a semantic scoring change — first-engagement vs any-engagement yields a different persisted observed_label/score for byte-identical agent traces. No lint or type gate can evaluate a scoring function on a trace or decide whether a score delta is an intended methodology change vs a regression; it needs recorded turn_records fed through the checker and value assertions. Prevents: Finding 2 (skill_triggered.py:129 scoring semantics change shifts activation-suite headline metrics — and can flip the suite_thresholds gate — for identical traces, with the trend consumer now living in the separate coder-eval-uipath repo).
• Add a TestEarlyStopWatcher case that feeds two AgentStartEvents ([_agent_start(), _agent_start(), ...]) and asserts elapsed_seconds is anchored to the FIRST start (the second AgentStart is a no-op, per the on_event docstring), exercising the currently-uncovered branch at early_stop.py:236 (if self._started_monotonic is None). Separately, tighten the coverage gate: pyproject already sets branch = true, but the only enforcement is a global line-coverage fail_under, so a single untaken branch never fails CI — add a branch-coverage floor (or a per-critical-module coverage check for orchestration/early_stop.py) so uncovered branches surface mechanically instead of relying on a reviewer to spot them. Why not static: Verifying retry-idempotent wall-clock stamping is a runtime-behavior assertion over a live event stream (elapsed_seconds anchored to the first of two AgentStartEvents); a lint/type gate cannot judge the branch's semantics. Branch coverage can flag the untaken branch, but it is a coverage gate, not a lint/type check, and it still cannot assert the branch is correct. Prevents: Finding 1 (tests/test_early_stop.py:892 — the documented second-AgentStart no-op branch at early_stop.py:236->277 is never exercised, so a regression that reset the wall-clock origin on a retry would go undetected in the elapsed_seconds telemetry).

Top 5 Priority Actions

1. Treat the skill_triggered any-engagement→first-engagement change (src/coder_eval/criteria/skill_triggered.py:129) as a methodology break: re-score or annotate all historical activation-suite accuracy/recall/F1/confusion at this commit so the dashboard/trend consumer in the eval-runner repo does not misread the step-change as an agent regression.
2. Add regression tests locking the new first-engagement semantics for the two score-flipping trajectories (target-skill-first-then-competitor and wrong-skill-first-then-target) so any future change to src/coder_eval/criteria/skill_triggered.py:129 that would shift a persisted observed_label/score is caught.
3. Make _first_engaged_skill_names self-contained by sorting on sequence_number internally (src/coder_eval/criteria/skill_triggered.py:75) so first-engagement scoring correctness no longer depends on the unenforced caller-ordering invariant that a future call site could violate and silently mis-score.
4. Document the scoring blast radius (any→first engagement) explicitly in the PR description/changelog, since the diff omits that identical traces engaging multiple skills now persist different observed_label/score.
5. Add a TestEarlyStopWatcher case that feeds two AgentStartEvents and asserts elapsed_seconds stays anchored to the first start, covering the documented retry-idempotent stamp branch (early_stop.py:261) that tests/test_early_stop.py:892 currently leaves unexercised.

---

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

[mohsen-uipath]

@bai-uipath — thanks for the thorough review; the tool-call-vs-result root-cause framing is spot on.

CR-1 (first-engagement → expected-skill latch) — good catch, thank you. Resolved. Both the final scorer and the live verdict now decide on any engagement (union over the trajectory) instead of first-engagement, so a wrong skill touched first no longer fails the row. Recovers uipath-agents-007 and uipath-maestro-case-038.

CR-2 (keep the negative side honest) — resolved; here's how the stacked criteria score. Each row stacks one skill_triggered criterion per candidate skill:

• GT criterion (skill_name == expected_skill) → recall: passes iff the expected skill was engaged anywhere.
• Every other criterion (skill_name != expected_skill, including all criteria on a negative row where expected_skill == "") → precision: fails iff that skill was engaged anywhere.

So a positive row that also fires an off-target skill still takes the precision hit on that skill's own confusion cell, and a negative row fails on any engagement — no precision hole. Arming is honest too: a positive can only live-pass, a distractor/negative only live-fail, so a mismatched stop_when is rejected at resolution rather than silently degrading to a full run.

F-3 (watcher fires on file-read, not just a Skill call) — confirmed + tested. The watcher latches on a Read/Grep/Bash ToolStartEvent whose params contain skills/<name>/, not only a Claude Skill call, so early-stop fires for antigravity/codex too.

CR-3 / F-1 / F-2 / F-4 — agreed, all outside this PR; I'll follow up after the skills side lands. These belong with the skills work in UiPath/skills#2028, which I'll merge first and then follow up on:

• CR-3 — add the uipath-connector-builder dataset + stacked criterion to the activation suite.
• F-1 — arm the activation criteria there. Note the coupling with this PR: under the new decidability, arm positives with stop_when: pass and distractors/negatives with stop_when: fail — not decided (a mismatched arm is now rejected at resolution).
• F-2 — the antigravity max_turns no-op is a separate coder_eval fix; I'll track it on its own.
• F-4 — turn-0 timeouts are run-config (higher task_timeout / lower parallelism), tracked separately.

[mohsen-uipath]

@uipreliga — thanks for the thorough pass. Key context: this review is against d1cb685, before the follow-up 0b093b1, which restored any-engagement (shared _all_engaged_skill_names, a set-union over the trajectory). That inverts/moots the ordering-based findings:

• 🟠 Methodology break — neutralized: scoring is net-zero vs pre-PR main, so nothing to backfill/annotate.
• _first_engaged_skill_names ordering + CE026 — moot: that function is gone; the set-union is order-independent, so there's no "first" to mis-pick.

Addressed in 603e23c:

• Regression tests (Priority chore: Bump actions/setup-node from 4.4.0 to 6.4.0 #2) — golden-corpus pinning observed_label + score for the canonical trajectories, so any future policy change fails loudly.
• Codex/multi-turn parity — any-engagement asserted across turns and for the file-read signal, not just the Claude Skill call.
• Two-AgentStart (Priority Bump next from 15.5.15 to 15.5.18 in /evalboard #5) — asserts the second AgentStart doesn't reset the wall-clock origin.
• tool_call_index off-by-one — field doc clarified: on a call-latched stop it's the deciding in-flight call (completed_tool_ends + 1), i.e. "which call decided", not a completed count.

The in-flight re-sort test is skipped intentionally — order no longer affects scoring.