fix(cli-called): report a zero-match max_count: 0 guard as unproven - #205
Conversation
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>
33c8a4f to
75cb56e
Compare
uipreliga
left a comment
There was a problem hiding this comment.
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
- [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 addsif 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_implkeeps only the refuse-to-score paths and the count, and the next wording change does not add more branching to the method. n/a - [Axis 1] Redundant
count == 0term in the unproven-guard predicate (src/coder_eval/criteria/cli_called.py:173) — Inif score == 1.0 and count == 0 and criterion.max_count == 0:,score == 1.0requireswithin_upper, which iscount <= criterion.max_count. Withmax_count == 0and a non-negative count, that already meanscount == 0, so the middle term can never change the result. It also costs one of the +3 CC. Shorten it toif 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 - [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 namepositional. That facet goes intowantedat 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 likef"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 - [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 isif 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 assertsresult.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 formin_count=0, max_count=1with 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. - [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 addsif 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. Itsmin_count: 0, max_count: 0form still reportsf"{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 parsesdetails. 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 outbound, so a zero-match guard no longer echoesmin_count=0, max_count=0, and the conditioncount == 0is redundant, becausescore == 1.0together withmax_count == 0already implies it. - [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 branchif score == 1.0 and count == 0 and criterion.max_count == 0:emitsf"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 oldsatisfies {bound}text. The data that would tell the two cases apart is already in scope:mine(tool-scoped records) andusable, which thenot within_lowerbranch 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: onlydetailsdiffers, 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.pyas a whole-tree@pytest.mark.lintclass intests/test_custom_lint.py, because it reasons over the whole package and not one AST at a time. The rule runsradon.complexity.cc_visit; radon is already a runtime dependency. Radon counts boolean operators, which ruff's mccabe does not, so it sees the extraand count == 0term. The rule fails when any function insrc/coder_eval/criteria/has a radon CC above 20. Existing offenders go in aCEILINGSdict 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 toexternalin pyproject.toml. Record the defect in.claude/notes/lint-rules.md. Prevents: Finding 1:CliCalledChecker._check_implwent 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 redundantcount == 0conjunct (finding 2), because that conjunct adds +1 radon CC. It cannot prove the conjunct is redundant. Proving that needs range reasoning overscore/within_upper/max_count, which is beyond AST lint and pyright. - [ruff] Turn on
C901in[tool.ruff.lint] selectwith[tool.ruff.lint.mccabe] max-complexity = 20. This is repo-wide and sits beside the existing PLR0912/PLR0915 size ceilings.ruff --select C901finds 6 functions above 20 today (max 29,DockerRunner._build_argv). Give each one a visible# noqa: C901debt marker, as the pyproject comment already does for PLR0915/PLR0912. This is the coarse backstop to CE067. Mccabe does not countand/or, so it could not catch finding 1 alone (ruff scores_check_implbelow 20). It stops the same drift in other modules. Prevents: Finding 1 as a class: branch growth in any function outsidecriteria/, for example the agentcommunicatemethods 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)detailscontains 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 asmax_count is not Nonevs== 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/wantedlist that already renderspositional=...and the value flags, for example 'a typo in any of: '. Add a test that sets each optionalCliCalledCriterionfacet 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 outpositionaland value-flag typos. - Add a shared negative-guard contract across checkers. Move the vacuous-guard caveat into a helper in
criteria/base.py, for examplevacuous_guard_note(recorded: int, sample: str) -> str. Add a parametrized contract test, in the style of CE036'sContractCases, over every registered criterion model with bothmin_countandmax_countfields (todaycli_calledandcommand_executed). For each model, amin_count: 0, max_count: 0criterion 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 thedetailsit 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_calledsays 'guard unproven' butcommand_executed.py:358still 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 thenot within_lowerbranch 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 theirdetailsstrings 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 samedetailsbytes, which is the original complaint in issue #122. - Add a periodic mutation-testing target,
make mutate-criteria, that runs mutmut oversrc/coder_eval/criteria/with the criteria tests. It should report surviving mutants in branch predicates. Make it a scheduled or nightly job, not amake verifygate. 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 ascriterion.max_count is not Noneor dropping themax_count == 0term. These pass today because only one side of the boundary is tested.
Top 5 Priority Actions
- 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 == 0and dropping the max_count term all pass the tests, so a bounded-range guard could be mislabeled 'guard unproven' and no test would fail. - 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 frommine/usable. Then a reviewer can tell a typo from a guard the agent respected. Update the'satisfies' not in detailsassertion in the test to match. - 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.
- 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_implthen keeps only the refuse-to-score paths and the count. - Remove the redundant
count == 0term from the predicate at src/coder_eval/criteria/cli_called.py:173, becausescore == 1.0andmax_count == 0already 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.
|
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. |
Why
A
max_count: 0guard that matches nothing scores 1.0 and reports0 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 inverb,toolor 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 == 0andmax_count == 0, the detail is now: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_guardnow also assertsguard unprovenand nosatisfies.test_positive_match_keeps_satisfies_wordingasserts 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(afteruv sync --frozen --extra dev --extra uipath --extra codex --extra litellm --extra harbor): ruff format/check clean, pyright 0 errors, custom lint 677 passed,prose_budgetclean, full suite 6093 passed / 6 failed / 4 skipped, coverage 92.48%. The 6 failures happen on unmodifiedmaintoo, in the same local environment:test_sandbox.py×2 (a/node_modulesdirectory at the filesystem root),test_stats_nonfinite.py×3 andtest_docker_runner_mounts.py×1. They are not related to this change.Closes #122