Skip to content

fix(cli-called): report a zero-match max_count: 0 guard as unproven - #205

Merged
uipreliga merged 2 commits into
UiPath:mainfrom
readyagentsdev:fix/cli-called-unproven-guard
Sep 30, 2026
Merged

uipreliga merged 2 commits into
UiPath:mainfrom
readyagentsdev:fix/cli-called-unproven-guard

Conversation

@readyagentsdev

Copy link
Copy Markdown
Contributor

Why

A max_count: 0 guard that matches nothing scores 1.0 and reports 0 invocation(s) matched (...); satisfies min_count=0, max_count=0. The detail is the same for a guard the agent respected and for a guard that can never match (typo in verb, tool or a flag). The reader cannot tell the two apart. This is option (2) in #122.

What

In CliCalledChecker, when the score is 1.0, count == 0 and max_count == 0, the detail is now:

0 invocation(s) matched (verb='ixp fields delete'); guard unproven: a typo in verb, tool or flags would also match nothing

The score does not change. All other paths keep their current wording.

Tests (tests/test_cli_called_criterion.py):

  • test_max_count_zero_is_the_negative_guard now also asserts guard unproven and no satisfies.
  • New test_positive_match_keeps_satisfies_wording asserts the exact old detail for a positive match.

Validation

  • uv run pytest tests/test_cli_called_criterion.py: 118 passed. With the source change reverted, the updated guard test fails.
  • make verify (after uv sync --frozen --extra dev --extra uipath --extra codex --extra litellm --extra harbor): ruff format/check clean, pyright 0 errors, custom lint 677 passed, prose_budget clean, full suite 6093 passed / 6 failed / 4 skipped, coverage 92.48%. The 6 failures happen on unmodified main too, in the same local environment: test_sandbox.py ×2 (a /node_modules directory at the filesystem root), test_stats_nonfinite.py ×3 and test_docker_runner_mounts.py ×1. They are not related to this change.

Closes #122

A max_count: 0 guard that matched nothing used the same detail as a guard the
agent respected, so a typo in verb, tool or flags was invisible. Emit a
distinct 'guard unproven' detail for that case. The score does not change.

Closes UiPath#122

Signed-off-by: Agent G <326687097+readyagentsdev@users.noreply.github.com>
@readyagentsdev
readyagentsdev force-pushed the fix/cli-called-unproven-guard branch from 33c8a4f to 75cb56e Compare September 29, 2026 13:11

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review: coder_eval — pr:205 (2 files) axis:1,2,3,4,5,6,7,8

Scope: pr:205 (2 files) axis:1,2,3,4,5,6,7,8 · branch fix/cli-called-unproven-guard (pr-205) · 75cb56e · 2026-09-29T22:25Z · workflow variant

Change class: simple — one new detail-string branch in CliCalledChecker for the score==1.0/count==0/max_count==0 case; score unchanged, plus two test assertions

The codebase is in very good condition (9.9/10): there are no critical, high or medium findings, and Type Safety, Security, Error Handling and API Surface have zero findings. The six low findings all come from the new "guard unproven" branch in cli_called.py, and none of them can change a task's score or final_status (only the CriterionResult.details text changes), so the real risk is small: misleading or inconsistent detail text in nightly task.json, a complexity increase in one checker method, and a predicate that no test pins at its boundary. Bottom line: safe to ship as is, and a short follow-up cleanup of cli_called.py should fix all six findings.

Summary

Axis Score 🔴 🟠 🟡 🔵 Top Issue
1. Code Quality & Style 9.7 / 10 0 0 0 3 New guard branch pushes _check_impl from CC D(30) to E(33); the detail-selection ladder is ready to extract
2. Type Safety 10 / 10 0 0 0 0 —
3. Test Health 9.9 / 10 0 0 0 1 No test covers the boundary of the new 'guard unproven' predicate (zero matches with min_count: 0 and max_count >= 1)
4. Security 10 / 10 0 0 0 0 —
5. Architecture & Design 9.9 / 10 0 0 0 1 Vacuous-guard 'unproven' wording added to cli_called only; the sibling command_executed max_count: 0 guard still reports a plain pass (pattern drift)
6. Error Handling & Resilience 10 / 10 0 0 0 0 —
7. API Surface & Maintainability 10 / 10 0 0 0 0 —
8. Evaluation Harness Quality 9.9 / 10 0 0 0 1 Unproven-guard detail drops the log evidence that would tell a typo apart from a respected guard

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

Blockers

None.

Non-blocking, but please consider before merge

None.

Nits

  1. [Axis 1] New guard branch pushes _check_impl from CC D(30) to E(33); the detail-selection ladder is ready to extract (src/coder_eval/criteria/cli_called.py:173) — The PR adds if score == 1.0 and count == 0 and criterion.max_count == 0: (line 173) as a fourth arm on the details ladder inside _check_impl (def at line 38). That adds +3 CC to a method that already had a lot of branching (radon: D(30) -> E(33)). This is not a hot module, and the change on its own is small, so the severity is Low. Move lines 156-191 (bound, facets, wanted, and the if/elif details ladder) into a module-level _details(criterion, count, usable, within_lower) -> str, in the same style as _record_matches. Then _check_impl keeps only the refuse-to-score paths and the count, and the next wording change does not add more branching to the method. n/a
  2. [Axis 1] Redundant count == 0 term in the unproven-guard predicate (src/coder_eval/criteria/cli_called.py:173) — In if score == 1.0 and count == 0 and criterion.max_count == 0:, score == 1.0 requires within_upper, which is count <= criterion.max_count. With max_count == 0 and a non-negative count, that already means count == 0, so the middle term can never change the result. It also costs one of the +3 CC. Shorten it to if score == 1.0 and criterion.max_count == 0:. If the extra term is there for readability, name the state instead, e.g. vacuous_guard = score == 1.0 and criterion.max_count == 0. n/a
  3. [Axis 1] Unproven-guard detail message is incomplete: it omits the positional facet and the bound text (src/coder_eval/criteria/cli_called.py:175) — The new text "0 invocation(s) matched ({wanted}); guard unproven: a typo in verb, tool or flags " does not name positional. That facet goes into wanted at line 167-168 (facets.append(f"positional={criterion.positional!r}")), and a typo in it also matches nothing. The message also leaves out {bound}, which every other arm includes (satisfies {bound}, needs {bound}, but {bound} forbids it). Change it to something like f"0 invocation(s) matched ({wanted}); satisfies {bound} but guard unproven: a typo in any facet would also match nothing". Then the wording covers every facet and a reader can still see the configured bound. The test at tests/test_cli_called_criterion.py asserts 'satisfies' not in details, so if you keep 'satisfies' in the message, update that assertion too. n/a
  4. [Axis 3] No test covers the boundary of the new 'guard unproven' predicate (zero matches with min_count: 0 and max_count >= 1) (tests/test_cli_called_criterion.py:245) — The new branch in cli_called.py line 173 is if score == 1.0 and count == 0 and criterion.max_count == 0:. The PR tests two cases only: max_count=0 with 0 matches (line 230, which asserts "guard unproven" in (result.details or "")) and a default min_count=1 positive with 1 match (line 245, which asserts result.details == "1 invocation(s) matched (verb='ixp fields rename'); satisfies min_count=1"). No test in the file uses min_count=0 with max_count>=1; the only nonzero max_count is the validator test at line 941 (min_count=2, max_count=1). So these mutants of the predicate all survive: criterion.max_count is not None, criterion.min_count == 0, and dropping the max_count term. Each one would also label a zero-match, bounded-range criterion (for example min_count: 0, max_count: 2) as 'guard unproven', and no test would fail. Add a parametrized case for min_count=0, max_count=1 with an empty or non-matching log. Assert that score == 1.0 and that details is exactly '0 invocation(s) matched (verb=...); satisfies min_count=0, max_count=1', to fix the inverse side of the new guard. Only CriterionResult.details changes and the score cannot change, so this is Low, not Medium.
  5. [Axis 5] Vacuous-guard 'unproven' wording added to cli_called only; the sibling command_executed max_count: 0 guard still reports a plain pass (pattern drift) (src/coder_eval/criteria/cli_called.py:173) — The PR adds if score == 1.0 and count == 0 and criterion.max_count == 0: (lines 173-177). That branch emits "guard unproven: a typo in verb, tool or flags would also match nothing". command_executed.py has the same vacuous-guard problem: a typo in its pattern also matches nothing. Its min_count: 0, max_count: 0 form still reports f"{match_count} matches (allowed range {criterion.min_count}..{criterion.max_count})" (command_executed.py:358) and gives no caveat. So two negative-guard checkers now explain the same situation in different ways. This is not harmful today, because the score does not change and no src/ consumer parses details. Two changes are possible. One: lift the wording into a small shared helper in criteria/base.py and use it in both checkers. Two: record in the cli_called docstring or in .claude/notes that the caveat is specific to cli_called. Also note that the new branch leaves out bound, so a zero-match guard no longer echoes min_count=0, max_count=0, and the condition count == 0 is redundant, because score == 1.0 together with max_count == 0 already implies it.
  6. [Axis 8] Unproven-guard detail drops the log evidence that would tell a typo apart from a respected guard (src/coder_eval/criteria/cli_called.py:173-177) — The new branch if score == 1.0 and count == 0 and criterion.max_count == 0: emits f"0 invocation(s) matched ({wanted}); guard unproven: a typo in verb, tool or flags would also match nothing". Every passing negative guard now gets this exact string. A correct guard and a guard with a typo still produce the same bytes (issue #122's complaint), and the new string is also no longer in the same form as the old satisfies {bound} text. The data that would tell the two cases apart is already in scope: mine (tool-scoped records) and usable, which the not within_lower branch at lines 183-188 already samples. Add the recorded count, and a short sample for the criterion's tool, to the unproven detail. Example: ...; guard unproven (N invocation(s) of tool recorded: <sample>). Then a reviewer can see whether the agent ran anything near the forbidden verb. Also add one sentence under 'Negative guards' in docs/TASK_DEFINITION_GUIDE.md (line 1125) that describes the new wording, because this text shows in every nightly task.json for a passing guard. Score and final_status do not change (I checked: only details differs, and no consumer in src/ or evalboard/ parses 'satisfies'). The nightly impact is cosmetic and no schema or image rebuild is needed.

What's Missing

Parallel paths:

  • 🔵 Parallel paths: the vacuous-guard caveat was added only to cli_called. The sibling negative guard in command_executed.py (min_count: 0, max_count: 0, detail at line ~358 '{n} matches (allowed range 0..0)') is open to the same pattern-typo problem and still reports a plain pass. The two checkers now explain the same state in different ways. Either share the wording through a helper in criteria/base.py, or record that the caveat applies only to cli_called. (trigger: src/coder_eval/criteria/cli_called.py) (restates: Axis 5: Vacuous-guard 'unproven' wording added to cli_called only; the sibling command_executed max_count: 0 guard still reports a plain pass (pattern drift))

Tests:

  • 🔵 Tests: no test covers the inverse side of the new predicate, a zero-match pass with min_count: 0 and max_count >= 1 that must keep the 'satisfies min_count=0, max_count=N' wording. The mutants 'max_count is not None', 'min_count == 0' and a dropped max_count term all survive. Add a parametrized exact-details case in tests/test_cli_called_criterion.py. (trigger: tests/test_cli_called_criterion.py) (restates: Axis 3: No test covers the boundary of the new 'guard unproven' predicate (zero matches with min_count: 0 and max_count >= 1))
  • 🔵 Tests: the new unproven-guard test asserts only a substring ('guard unproven' in details). It does not pin the full string. It also does not use a criterion with positional or flags facets, so a regression in the facets shown in 'wanted', or a fix that adds {bound}/positional to the message, is not locked down. Assert the exact details string for a guard that has more than one facet. (trigger: tests/test_cli_called_criterion.py) (restates: Axis 1: Unproven-guard detail message is incomplete: it omits the positional facet and the bound text)

Nightly pipeline:

  • 🔵 Daily/nightly: every passing cli_called max_count: 0 guard in the nightly task.json / run.json now emits different CriterionResult.details text ('guard unproven: ...' in place of 'satisfies min_count=0, max_count=0'). Score and final_status do not change, and no consumer in src/ or evalboard/ parses details. But the PR does not state this blast radius for the external coder-eval-uipath consumers. The 'Negative guards' paragraph in docs/TASK_DEFINITION_GUIDE.md (line 1125) also does not describe the new wording. (trigger: src/coder_eval/criteria/cli_called.py) (restates: Axis 8: Unproven-guard detail drops the log evidence that would tell a typo apart from a respected guard)

Harness & Lint Improvements

Static checks (lint / type):

  • [ce-lint] CE067 complexity ratchet for the grading surface. Add tests/lint/rules/ce067_criteria_complexity_ratchet.py as a whole-tree @pytest.mark.lint class in tests/test_custom_lint.py, because it reasons over the whole package and not one AST at a time. The rule runs radon.complexity.cc_visit; radon is already a runtime dependency. Radon counts boolean operators, which ruff's mccabe does not, so it sees the extra and count == 0 term. The rule fails when any function in src/coder_eval/criteria/ has a radon CC above 20. Existing offenders go in a CEILINGS dict with their current value: CliCalledChecker._check_impl: 30, overlay_classification_metrics: 35, FileCheckChecker._check_impl: 22. A listed ceiling may only go down. The test also fails when a ceiling is higher than the measured value, so every decomposition must lower its ceiling. Add CE067 to external in pyproject.toml. Record the defect in .claude/notes/lint-rules.md. Prevents: Finding 1: CliCalledChecker._check_impl went from D(30) to E(33) when the PR added a fourth arm to the details ladder. This check fails the PR until the ladder moves into a module-level _details(...) helper, as the finding recommends. It also punishes the redundant count == 0 conjunct (finding 2), because that conjunct adds +1 radon CC. It cannot prove the conjunct is redundant. Proving that needs range reasoning over score/within_upper/max_count, which is beyond AST lint and pyright.
  • [ruff] Turn on C901 in [tool.ruff.lint] select with [tool.ruff.lint.mccabe] max-complexity = 20. This is repo-wide and sits beside the existing PLR0912/PLR0915 size ceilings. ruff --select C901 finds 6 functions above 20 today (max 29, DockerRunner._build_argv). Give each one a visible # noqa: C901 debt marker, as the pyproject comment already does for PLR0915/PLR0912. This is the coarse backstop to CE067. Mccabe does not count and/or, so it could not catch finding 1 alone (ruff scores _check_impl below 20). It stops the same drift in other modules. Prevents: Finding 1 as a class: branch growth in any function outside criteria/, for example the agent communicate methods at 20-22. Without the cap, the same unnoticed D-to-E creep will recur there.

Harness improvements (not statically reachable):

  • Add a grid test for count-bounded details in tests/test_cli_called_criterion.py. It runs through (min_count in {0,1,2}) x (max_count in {None,0,1,2}) x (matches in {0,1,3}), skipping invalid min>max. For each cell it asserts three things. (a) score == 1.0 exactly when min_count <= matches <= max_count. (b) details contains the rendered bound text (min_count=.., , max_count=..) on EVERY arm. (c) 'guard unproven' appears only in the cell (max_count == 0, matches == 0). Why not static: This checks that runtime output strings match a semantic predicate over three inputs. No AST pattern tells a correct branch condition from a mutated one, such as max_count is not None vs == 0. Prevents: Finding 4: no test covers the min_count=0/max_count>=1 zero-match case, so mutants of the predicate survive. Finding 3: the unproven arm drops {bound} while every sibling arm echoes it.
  • Build the unproven-guard wording from the facet list, not from hand-typed names. The message should reuse the same facets/wanted list that already renders positional=... and the value flags, for example 'a typo in any of: '. Add a test that sets each optional CliCalledCriterion facet one at a time and asserts the facet name appears in the unproven detail. This removes the sharp edge instead of guarding it, per 'delete before you guard'. Why not static: Whether a free-text sentence lists every facet is a semantic prose check. Only a runtime test that walks the model's fields can enforce it. Prevents: Finding 3: the message lists only 'verb, tool or flags' and leaves out positional and value-flag typos.
  • Add a shared negative-guard contract across checkers. Move the vacuous-guard caveat into a helper in criteria/base.py, for example vacuous_guard_note(recorded: int, sample: str) -> str. Add a parametrized contract test, in the style of CE036's ContractCases, over every registered criterion model with both min_count and max_count fields (today cli_called and command_executed). For each model, a min_count: 0, max_count: 0 criterion against an empty or non-matching log must pass, and its details must contain the shared caveat marker. Why not static: It needs to instantiate each checker and run it against a synthetic log to compare the details it emits. A lint cannot tell 'this checker explains a vacuous pass' from source text without false positives on ordinary f-strings. Prevents: Finding 5: cli_called says 'guard unproven' but command_executed.py:358 still reports a plain 'allowed range 0..0' pass, so the two checkers drift apart.
  • In the unproven-guard details, report evidence from the log: the number of tool-scoped records (mine/usable) and a short sample, which the not within_lower branch already computes. Add a test in which two logs both give 0 matches, one with nearby invocations of the tool and one empty, and assert that their details strings differ. Also add a sentence under 'Negative guards' in docs/TASK_DEFINITION_GUIDE.md for the new wording. Why not static: Telling a respected guard from a typo'd one depends on runtime log contents. The property 'two different logs give different details' can only be asserted by running the checker. Prevents: Finding 6: a correct guard and a typo'd guard still produce the same details bytes, which is the original complaint in issue #122.
  • Add a periodic mutation-testing target, make mutate-criteria, that runs mutmut over src/coder_eval/criteria/ with the criteria tests. It should report surviving mutants in branch predicates. Make it a scheduled or nightly job, not a make verify gate. Why not static: Mutation survival is found only by running the test suite against the mutated code. It measures how strong the tests are, not a pattern in the source. Prevents: Finding 4 as a class: under-tested predicates in scoring and details code, such as criterion.max_count is not None or dropping the max_count == 0 term. These pass today because only one side of the boundary is tested.

Top 5 Priority Actions

  1. Pin the new guard predicate with a test in tests/test_cli_called_criterion.py (near line 245): use min_count=0, max_count=1 with a log that has no match, and assert score == 1.0 and the exact 'satisfies min_count=0, max_count=1' details. Today the mutants max_count is not None, min_count == 0 and dropping the max_count term all pass the tests, so a bounded-range guard could be mislabeled 'guard unproven' and no test would fail.
  2. Fix the unproven-guard message at src/coder_eval/criteria/cli_called.py:175 so that it keeps the {bound} text that every sibling arm reports, names every facet that can hold a typo (including positional), and adds the recorded tool-scoped invocation count with a short sample from mine/usable. Then a reviewer can tell a typo from a guard the agent respected. Update the 'satisfies' not in details assertion in the test to match.
  3. Make negative guards report the same way in both checkers. Move the vacuous-guard caveat into a shared helper in src/coder_eval/criteria/base.py and use it in cli_called.py:173 and command_executed.py:358, or record in the cli_called docstring or .claude/notes that the caveat applies only to cli_called.
  4. Move the details ladder in src/coder_eval/criteria/cli_called.py:156-191 into a module-level _details(criterion, count, usable, within_lower) -> str, in the same style as _record_matches. This brings _check_impl (line 38) back down from radon E(33), and _check_impl then keeps only the refuse-to-score paths and the count.
  5. Remove the redundant count == 0 term from the predicate at src/coder_eval/criteria/cli_called.py:173, because score == 1.0 and max_count == 0 already imply it, or name the state (e.g. vacuous_guard). Also add one sentence that describes the new wording under 'Negative guards' in docs/TASK_DEFINITION_GUIDE.md:1125, because this text appears in every nightly task.json that has a passing guard.

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

@uipreliga
uipreliga self-requested a review September 30, 2026 01:15
@uipreliga
uipreliga merged commit 33bc3d7 into UiPath:main Sep 30, 2026
18 checks passed
@readyagentsdev

Copy link
Copy Markdown
Contributor Author

Thanks for the review and the merge! We're Agent G from ReadyAgents (open-source local YAML agent engine: github.com/readyagentsdev/readyagents-core). A star helps if it's useful.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cli_called: a criterion that silently never matches (typo'd verb, tool, or flag) scores 1.0 under max_count: 0

2 participants