From de70dd0060801b05ec4e6bbbf765f63fefa080d0 Mon Sep 17 00:00:00 2001 From: Alexandru Jircan Date: Tue, 11 Aug 2026 16:30:13 +0300 Subject: [PATCH 01/12] feat(cli-called): accept a list of verb spellings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `verb` was a single string matched as an ordered prefix, so a criterion needing "list OR get" had one option: truncate to the common prefix. That leaves every following token unconstrained — safe for a max_count 0 guard, which then fires on more, but on a positive assertion it credits any sibling subcommand. A real case: `verb: "ixp projects"` on a weight-3.0 criterion asserting the agent read a project also credited `projects delete`, `projects update-title`, `projects publish` and a hallucinated `projects fetch-meta`. The regex it replaced said `(list|get)` and admitted none of them. The API made the unsafe option the only expressible one. `verb` now takes a string or a list; a list matches if any entry does. The docstring states the breadth asymmetry it previously left implicit — order was documented, widening was not. Validation, since each of these matches every record or scores by list order: - empty list rejected (falsy, so it slipped past the at-least-one-facet check and read as "no verb constraint") - blank entry rejected per item (`" ".split()` is an empty prefix) - one spelling being a prefix of another rejected: both match the same argv while consuming different token counts, so the `positional` offset would depend on order. Catches duplicates too, a prefix of themselves. Matching stays token-by-token, so `projects list` still never matches `projects lists` — now covered by a test, since that property is what makes listing full verbs sufficient. 27 new tests including the inverse (a negative guard must fire on EVERY listed spelling — a change that only widened the positive path would leave that green). Single-string detail rendering is byte-identical; lists render as `a | b`. Co-Authored-By: Claude Opus 5 (1M context) --- src/coder_eval/criteria/cli_called.py | 22 +++-- src/coder_eval/models/criteria.py | 60 +++++++++++-- tests/test_cli_called_criterion.py | 121 ++++++++++++++++++++++++++ 3 files changed, 186 insertions(+), 17 deletions(-) diff --git a/src/coder_eval/criteria/cli_called.py b/src/coder_eval/criteria/cli_called.py index 74532c68..68e11cd5 100644 --- a/src/coder_eval/criteria/cli_called.py +++ b/src/coder_eval/criteria/cli_called.py @@ -150,14 +150,18 @@ def _record_matches(criterion: CliCalledCriterion, argv: list[str], record: dict ) offset = 0 - if criterion.verb is not None: - verb_tokens = criterion.verb.split() - # ORDERED prefix, not a token subset: `labellings confirm` must never be - # satisfied by `labellings unconfirm`, and a project name that happens to - # equal a subcommand must not stand in for the subcommand. - if positional[: len(verb_tokens)] != verb_tokens: + spellings = criterion.verb_spellings + if spellings: + # ORDERED prefix compared token by token — not a subset, and not a string + # startswith: `labellings confirm` must never be satisfied by + # `labellings unconfirm`, nor `projects list` by `projects lists`. + matched = next((tokens for tokens in spellings if positional[: len(tokens)] == tokens), None) + if matched is None: return False - offset = len(verb_tokens) + # Offset comes from the candidate that matched, since spellings may differ in + # length. Validation rejects one spelling being a prefix of another, so at + # most one can match and this cannot depend on list order. + offset = len(matched) if criterion.positional is not None: expected = criterion.positional @@ -279,7 +283,9 @@ def _check_impl( if criterion.tool is not None: facets.append(f"tool={criterion.tool!r}") if criterion.verb is not None: - facets.append(f"verb={criterion.verb!r}") + # ' | ' rather than repr of the list: a bare list reads as "the verb is + # these tokens". A single verb renders exactly as it did before. + facets.append(f"verb={' | '.join(' '.join(t) for t in criterion.verb_spellings)!r}") if criterion.positional is not None: facets.append(f"positional={criterion.positional!r}") if criterion.flags: diff --git a/src/coder_eval/models/criteria.py b/src/coder_eval/models/criteria.py index e1bf0fcc..ff295927 100644 --- a/src/coder_eval/models/criteria.py +++ b/src/coder_eval/models/criteria.py @@ -576,13 +576,17 @@ class CliCalledCriterion(BaseSuccessCriterion): "generated recorders never repeats it" ), ) - verb: str | None = Field( + verb: str | list[str] | None = Field( default=None, - min_length=1, description=( "Whitespace-separated subcommand chain that must be an ORDERED PREFIX of the invocation's " - "non-flag arguments. Order matters, so 'labellings confirm' never matches " - "'labellings unconfirm'" + "non-flag arguments, compared token by token (so 'projects list' never matches " + "'projects lists'). Order matters, so 'labellings confirm' never matches " + "'labellings unconfirm'. A LIST matches if ANY entry does, for a verb the tool spells " + "several ways. Prefer listing full verbs over truncating one to cover several: a short " + "verb leaves the following tokens unconstrained, which is safe for a max_count 0 guard " + "(it fires on more) but NOT for a positive assertion, where 'projects' would credit " + "'projects delete' as readily as 'projects get'" ), ) tool: str | None = Field( @@ -632,6 +636,18 @@ class CliCalledCriterion(BaseSuccessCriterion): ), ) + @property + def verb_spellings(self) -> list[list[str]]: + """Each accepted verb as its token list; empty when there is no verb constraint. + + One place splits the field, so the validator and the checker cannot disagree + about what a spelling is. + """ + if self.verb is None: + return [] + spellings = [self.verb] if isinstance(self.verb, str) else self.verb + return [spelling.split() for spelling in spellings] + @model_validator(mode="after") def _validate_bounds(self) -> CliCalledCriterion: # min_count 0 with no upper bound is satisfied by every possible log, so @@ -645,11 +661,37 @@ def _validate_bounds(self) -> CliCalledCriterion: if self.max_count is not None and self.max_count < self.min_count: msg = f"max_count ({self.max_count}) must be >= min_count ({self.min_count})" raise ValueError(msg) - # min_length=1 counts characters, so " " passes it — and `" ".split()` - # is `[]`, an empty prefix that matches every record. - if self.verb is not None and not self.verb.strip(): - msg = "cli_called verb must not be blank: a blank verb is an empty prefix and matches every record" - raise ValueError(msg) + if self.verb is not None: + spellings = [self.verb] if isinstance(self.verb, str) else self.verb + # `verb: []` is falsy, so the "at least one facet" check below would let + # it through whenever positional/flags/tool is set — as "no verb + # constraint", quietly matching more than the author wrote. + if not spellings: + msg = "cli_called verb list must not be empty: drop the field to match any verb" + raise ValueError(msg) + # A character count would pass " ", and `" ".split()` is `[]` — an + # empty prefix that matches every record. + if any(not spelling.strip() for spelling in spellings): + msg = ( + "cli_called verb must not be blank: a blank verb is an empty prefix and matches " + "every record" + ) + raise ValueError(msg) + # One candidate being a prefix of another makes the match ambiguous: both + # accept the same argv but consume a different number of tokens, so the + # offset `positional` is measured from would depend on candidate order. + # Identical entries land here too, a prefix of itself. + token_lists = [spelling.split() for spelling in spellings] + for outer, shorter in enumerate(token_lists): + for inner, longer in enumerate(token_lists): + if outer != inner and longer[: len(shorter)] == shorter: + msg = ( + f"cli_called verb {' '.join(shorter)!r} is a prefix of " + f"{' '.join(longer)!r}; both would match the same invocation while " + "consuming a different number of tokens, making the `positional` offset " + "ambiguous. List only the verbs you mean, or keep the shorter one alone." + ) + raise ValueError(msg) # Falsiness-symmetric on purpose: `verb: ""` used to slip past an `is None` # check here and then match EVERY record (empty prefix), silently scoring 1.0. if not self.verb and not self.positional and not self.flags and not self.tool: diff --git a/tests/test_cli_called_criterion.py b/tests/test_cli_called_criterion.py index bda64b92..91bdd315 100644 --- a/tests/test_cli_called_criterion.py +++ b/tests/test_cli_called_criterion.py @@ -741,3 +741,124 @@ def test_criterion_with_nothing_to_match_rejected(self): def test_unknown_field_rejected(self): with pytest.raises(ValidationError, match="Extra inputs are not permitted"): CliCalledCriterion(description="d", log=LOG, verb="v", pattern="oops") + + +class TestVerbAlternation: + """A verb the tool spells several ways, e.g. the old regex's `(list|get)`. + + Without alternation the only way to accept two verbs was to truncate to their + common prefix, which leaves the following tokens unconstrained — safe for a + max_count 0 guard, but on a positive assertion it credits `projects delete` as + readily as `projects get`. + """ + + @pytest.mark.parametrize("subcommand", ["list", "get"]) + def test_any_listed_spelling_matches(self, sandbox_with_log, subcommand): + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", subcommand, "proj-1"])]) + criterion = CliCalledCriterion( + description="read the project", + log=LOG, + verb=["ixp projects list", "ixp projects get"], + ) + assert SuccessChecker(sandbox).check(criterion).score == 1.0 + + def test_an_unlisted_sibling_does_not_match(self, sandbox_with_log): + """The point of the feature: `delete` is not silently admitted.""" + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", "delete", "proj-1"])]) + criterion = CliCalledCriterion( + description="read the project", + log=LOG, + verb=["ixp projects list", "ixp projects get"], + ) + assert SuccessChecker(sandbox).check(criterion).score == 0.0 + + def test_spelling_is_compared_token_by_token(self, sandbox_with_log): + """`projects list` must not match `projects lists` or `projects list-models`. + + The match is an ordered prefix over TOKENS, not a string startswith, so a + typo'd or longer-named sibling shares no token with the verb. + """ + sandbox, sandbox_dir = sandbox_with_log + _write_log( + sandbox_dir, + [ + _call(["ixp", "projects", "lists"]), + _call(["ixp", "projects", "list-models"]), + ], + ) + criterion = CliCalledCriterion(description="listed", log=LOG, verb=["ixp projects list"]) + assert SuccessChecker(sandbox).check(criterion).score == 0.0 + + @pytest.mark.parametrize("subcommand", ["publish", "unpublish"]) + def test_negative_guard_fires_on_every_listed_spelling(self, sandbox_with_log, subcommand): + """The inverse: a max_count 0 guard must fail on ANY listed verb. + + A change that only widened what scores 1.0 would leave the positive tests + green while the guard quietly stopped firing. + """ + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", subcommand, "proj-1"])]) + criterion = CliCalledCriterion( + description="did not change published state", + log=LOG, + verb=["ixp projects publish", "ixp projects unpublish"], + min_count=0, + max_count=0, + ) + assert SuccessChecker(sandbox).check(criterion).score == 0.0 + + def test_positional_offset_follows_the_matched_spelling(self, sandbox_with_log): + """Spellings of differing length each measure `positional` from their own end.""" + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "fields", "delete", "proj-1"])]) + criterion = CliCalledCriterion( + description="deleted from the right project", + log=LOG, + verb=["ixp fields remove", "ixp fields delete"], + positional=["proj-1"], + ) + assert SuccessChecker(sandbox).check(criterion).score == 1.0 + + def test_failure_detail_renders_the_alternatives(self, sandbox_with_log): + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", "delete", "proj-1"])]) + criterion = CliCalledCriterion( + description="read the project", + log=LOG, + verb=["ixp projects list", "ixp projects get"], + ) + result = SuccessChecker(sandbox).check(criterion) + assert "ixp projects list | ixp projects get" in (result.details or "") + + def test_single_verb_detail_is_unchanged(self, sandbox_with_log): + """A plain string verb renders exactly as it did before this feature.""" + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", "delete", "proj-1"])]) + criterion = CliCalledCriterion(description="read", log=LOG, verb="ixp projects get") + result = SuccessChecker(sandbox).check(criterion) + assert "verb='ixp projects get'" in (result.details or "") + + +class TestVerbAlternationValidation: + def test_empty_list_rejected(self): + """`verb: []` is falsy, so it would slip past the at-least-one-facet check.""" + with pytest.raises(ValidationError, match="must not be empty"): + CliCalledCriterion(description="d", log=LOG, verb=[], positional=["proj-1"]) + + @pytest.mark.parametrize("blank", ["", " "]) + def test_blank_entry_rejected(self, blank): + with pytest.raises(ValidationError, match="must not be blank"): + CliCalledCriterion(description="d", log=LOG, verb=["ixp projects get", blank]) + + def test_spelling_that_is_a_prefix_of_another_rejected(self): + """Both match while consuming different token counts, so the `positional` + offset would depend on list order.""" + with pytest.raises(ValidationError, match="is a prefix of"): + CliCalledCriterion(description="d", log=LOG, verb=["ixp projects", "ixp projects list"]) + + def test_duplicate_spellings_rejected(self): + """A duplicate is a prefix of itself, caught by the same rule.""" + with pytest.raises(ValidationError, match="is a prefix of"): + CliCalledCriterion(description="d", log=LOG, verb=["ixp projects get", "ixp projects get"]) From 5623313d92c7a36498c89af833ebcc55ecb8dd6e Mon Sep 17 00:00:00 2001 From: Alexandru Jircan Date: Tue, 11 Aug 2026 16:35:26 +0300 Subject: [PATCH 02/12] feat(cli-called): add exact_positional to pin the argument tail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Alternation fixed which subcommand matched; the tail stayed open. `verb: "ixp projects list"` also matched `ixp projects list dummy`, crediting an invocation the real CLI rejects — `positional` is a prefix, so anything past it is unconstrained. `positional: []` looked like the way to say "took no arguments" and was a silent no-op: an empty slice equals an empty expectation. It is now meaningful when paired with exact_positional, and exact_positional without positional is rejected so "exactly nothing" stays distinct from "unset". Flags are unaffected — only non-flag arguments count, so `--output json` never breaks an exact match. The asymmetry runs OPPOSITE to a short verb's, and is asserted rather than left to be discovered: widening is safe on a max_count 0 guard and unsafe on a positive assertion, while tightening is safe on a positive assertion and unsafe on a guard, where one stray argument stops the match and the forbidden call slips past. Both directions are now documented on the fields and pinned by tests. Default is unchanged, with a test recording it so a future change to the default fails loudly instead of silently retightening every existing criterion. 95 in the criterion file, 4027 in the full suite (same 8 pre-existing failures), make lint 177. Co-Authored-By: Claude Opus 5 (1M context) --- src/coder_eval/criteria/cli_called.py | 7 +- src/coder_eval/models/criteria.py | 26 +++++- tests/test_cli_called_criterion.py | 113 ++++++++++++++++++++++++++ 3 files changed, 144 insertions(+), 2 deletions(-) diff --git a/src/coder_eval/criteria/cli_called.py b/src/coder_eval/criteria/cli_called.py index 68e11cd5..a7257426 100644 --- a/src/coder_eval/criteria/cli_called.py +++ b/src/coder_eval/criteria/cli_called.py @@ -167,6 +167,10 @@ def _record_matches(criterion: CliCalledCriterion, argv: list[str], record: dict expected = criterion.positional if positional[offset : offset + len(expected)] != expected: return False + # Otherwise the match is a prefix: `projects list` accepts + # `projects list dummy`, crediting a malformed invocation. + if criterion.exact_positional and len(positional) != offset + len(expected): + return False if criterion.flags: for name, predicate in criterion.flags.items(): @@ -287,7 +291,8 @@ def _check_impl( # these tokens". A single verb renders exactly as it did before. facets.append(f"verb={' | '.join(' '.join(t) for t in criterion.verb_spellings)!r}") if criterion.positional is not None: - facets.append(f"positional={criterion.positional!r}") + exact = " exactly" if criterion.exact_positional else "" + facets.append(f"positional{exact}={criterion.positional!r}") if criterion.flags: facets.append(f"flags={sorted(criterion.flags)}") wanted = ", ".join(facets) diff --git a/src/coder_eval/models/criteria.py b/src/coder_eval/models/criteria.py index ff295927..58657aa8 100644 --- a/src/coder_eval/models/criteria.py +++ b/src/coder_eval/models/criteria.py @@ -595,7 +595,21 @@ class CliCalledCriterion(BaseSuccessCriterion): ) positional: list[str] | None = Field( default=None, - description="Non-flag arguments that must follow the verb, in order", + description=( + "Non-flag arguments that must follow the verb, in order. A PREFIX of what followed: " + "trailing arguments beyond these are unconstrained unless exact_positional is set" + ), + ) + exact_positional: bool = Field( + default=False, + description=( + "Require the non-flag arguments after the verb to be EXACTLY `positional`, with nothing " + "trailing. Without it `verb: 'projects list'` also matches `projects list dummy`. Set it " + "with `positional: []` to assert the verb took no arguments at all. Note the asymmetry " + "runs opposite to a short verb's: tightening suits a positive assertion, but on a " + "max_count 0 guard it makes the forbidden call EASIER to slip past, since one stray " + "argument stops the match" + ), ) flags: dict[str, FlagMatch] | None = Field( default=None, @@ -692,6 +706,16 @@ def _validate_bounds(self) -> CliCalledCriterion: "ambiguous. List only the verbs you mean, or keep the shorter one alone." ) raise ValueError(msg) + # `positional: []` alone asserts nothing (an empty slice equals an empty + # expectation), so an author writing it to mean "took no arguments" gets a + # silent no-op. exact_positional is what gives it meaning, and requiring the + # pair keeps "exactly nothing" distinct from "unset". + if self.exact_positional and self.positional is None: + msg = ( + "cli_called exact_positional requires positional to be set. Use `positional: []` to " + "assert the verb took no arguments." + ) + raise ValueError(msg) # Falsiness-symmetric on purpose: `verb: ""` used to slip past an `is None` # check here and then match EVERY record (empty prefix), silently scoring 1.0. if not self.verb and not self.positional and not self.flags and not self.tool: diff --git a/tests/test_cli_called_criterion.py b/tests/test_cli_called_criterion.py index 91bdd315..7ae8e0f6 100644 --- a/tests/test_cli_called_criterion.py +++ b/tests/test_cli_called_criterion.py @@ -841,6 +841,119 @@ def test_single_verb_detail_is_unchanged(self, sandbox_with_log): assert "verb='ixp projects get'" in (result.details or "") +class TestExactPositional: + """`positional` is a prefix, so trailing arguments are unconstrained by default. + + That credits a malformed invocation: `verb: 'projects list'` matches + `projects list dummy`, which the real CLI would reject. + """ + + def test_trailing_arguments_are_accepted_by_default(self, sandbox_with_log): + """Documents the default, so a change to it fails here rather than silently.""" + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", "list", "dummy"])]) + criterion = CliCalledCriterion(description="listed", log=LOG, verb="ixp projects list") + assert SuccessChecker(sandbox).check(criterion).score == 1.0 + + def test_exact_positional_rejects_trailing_arguments(self, sandbox_with_log): + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", "list", "dummy"])]) + criterion = CliCalledCriterion( + description="listed", + log=LOG, + verb="ixp projects list", + positional=[], + exact_positional=True, + ) + assert SuccessChecker(sandbox).check(criterion).score == 0.0 + + def test_exact_positional_accepts_the_bare_verb(self, sandbox_with_log): + """The inverse of the above: tightening must not reject the correct call.""" + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", "list"])]) + criterion = CliCalledCriterion( + description="listed", + log=LOG, + verb="ixp projects list", + positional=[], + exact_positional=True, + ) + assert SuccessChecker(sandbox).check(criterion).score == 1.0 + + def test_exact_positional_rejects_extra_beyond_a_listed_argument(self, sandbox_with_log): + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", "get", "proj-1", "proj-2"])]) + criterion = CliCalledCriterion( + description="read one project", + log=LOG, + verb="ixp projects get", + positional=["proj-1"], + exact_positional=True, + ) + assert SuccessChecker(sandbox).check(criterion).score == 0.0 + + def test_exact_positional_ignores_flags(self, sandbox_with_log): + """Only NON-flag arguments count, so `--output json` must not break it.""" + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", "get", "proj-1", "--output", "json"])]) + criterion = CliCalledCriterion( + description="read one project", + log=LOG, + verb="ixp projects get", + positional=["proj-1"], + exact_positional=True, + ) + assert SuccessChecker(sandbox).check(criterion).score == 1.0 + + def test_a_negative_guard_is_easier_to_evade_with_exact_positional(self, sandbox_with_log): + """The asymmetry, asserted so it is visible rather than discovered later. + + Tightening suits a positive assertion. On a max_count 0 guard it works the + other way: one stray argument stops the match, so the forbidden call slips + past. Documented on the field; pinned here. + """ + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", "delete", "proj-1", "stray"])]) + loose = CliCalledCriterion( + description="did not delete", + log=LOG, + verb="ixp projects delete", + min_count=0, + max_count=0, + ) + tight = CliCalledCriterion( + description="did not delete", + log=LOG, + verb="ixp projects delete", + positional=["proj-1"], + exact_positional=True, + min_count=0, + max_count=0, + ) + assert SuccessChecker(sandbox).check(loose).score == 0.0 + assert SuccessChecker(sandbox).check(tight).score == 1.0 + + def test_detail_marks_the_match_as_exact(self, sandbox_with_log): + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", "list", "dummy"])]) + criterion = CliCalledCriterion( + description="listed", + log=LOG, + verb="ixp projects list", + positional=[], + exact_positional=True, + ) + result = SuccessChecker(sandbox).check(criterion) + assert "positional exactly=[]" in (result.details or "") + + def test_exact_positional_without_positional_rejected(self): + """`positional: []` is the explicit way to say "no arguments".""" + with pytest.raises(ValidationError, match="requires positional to be set"): + CliCalledCriterion( + description="d", log=LOG, verb="ixp projects list", exact_positional=True + ) + + class TestVerbAlternationValidation: def test_empty_list_rejected(self): """`verb: []` is falsy, so it would slip past the at-least-one-facet check.""" From 90034314ee651eb09e04a0d748498abee787182c Mon Sep 17 00:00:00 2001 From: Alexandru Jircan Date: Wed, 12 Aug 2026 12:32:55 +0300 Subject: [PATCH 03/12] fix(cli-called): move alternation to verb_any_of, close review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #103 found the list arm of `verb: str | list[str]` reintroduced the very fail-open this PR set out to close. `verb: ["ixp", "projects", "list"]` — the natural way to mistype a chain — parsed as three single-token ALTERNATIVES, and the bare `ixp` entry is a one-token prefix matching every uip call, so it scored 1.0 on `ixp projects delete`. Reproduced before fixing. No validator can separate that from a legitimate `["list", "ls"]`, so the shape is gone from the schema: `verb` is a plain `str` again and alternation lives in `verb_any_of`, making the mistyped form a pydantic type error. Also from the review, each reproduced first: - The offset-from-matched-spelling test used two 3-token spellings, so the branch was never discriminated — mutating `offset = len(matched)` to `len(spellings[0])` left all 95 tests green. Now parametrized over genuinely differing lengths (`["ixp projects get", "ixp get"]`), both spellings exercised, plus the inverse and an order-independence case. The mutation now kills two tests. - `exact_positional` silently depends on `value_flags` completeness: an undeclared flag is read as a switch, so `--folder Finance` leaves its VALUE among the positionals and turns an exactly-correct invocation into 0.0. Stated on the field and pinned by a test asserting both directions. The previous test used `--output`, which is in both default lists — the benign direction. - `not self.positional` conflated `positional: []` with unset, rejecting `positional: [] + exact_positional` — the field's own documented headline use — as "requires at least one of ...". - The duplicate-entry message read "'a b' is a prefix of 'a b' ... keep the shorter one alone", which reads as a validator bug. Duplicates get their own message, and the prefix message no longer cites a `positional` the config may not have set. - `ruff format --check` was red on two files: `make format` had only ever been run over a narrower path set than `make verify` lints. Verb rules moved to their own `_validate_verb` so `_validate_bounds` stops growing, and both it and the failure detail now read `verb_spellings`, making that property's "one place splits the field" docstring true. Detail rendering normalizes whitespace, now stated in the comment and covered by a parametrized test rather than claimed to be identical. Docs: TASK_DEFINITION_GUIDE.md § cli_called gains `verb_any_of`, `exact_positional`, both hazard directions and the `value_flags` prerequisite; the CLAUDE.md criteria row names both new fields. 104 in the criterion file, 4036 in the full suite (same 8 pre-existing failures), make lint 177, ruff format/check clean. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 2 +- docs/TASK_DEFINITION_GUIDE.md | 18 ++- src/coder_eval/criteria/cli_called.py | 9 +- src/coder_eval/models/criteria.py | 125 +++++++++++-------- tests/test_cli_called_criterion.py | 171 +++++++++++++++++++++----- 5 files changed, 237 insertions(+), 88 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index a1402582..ae4331f0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -156,7 +156,7 @@ action.yml # Published composite GitHub Action (coder-ev | `file_matches_regex` | Binary | Regex match on file | | `reference_comparison` | Continuous | AST/token/complexity similarity | | `command_executed` | Fractional | Agent tool usage verification | -| `cli_called` | Binary | Structured match over a JSON Lines invocation log: verb / positional / per-flag predicates, with min_count/max_count bounds | +| `cli_called` | Binary | Structured match over a JSON Lines invocation log: verb (or `verb_any_of` alternation) / positional (optionally `exact_positional`) / per-flag predicates, with min_count/max_count bounds | | `commands_efficiency` | Continuous | Agent tool-call efficiency relative to expected budget | | `uipath_eval` | Fractional | UiPath agent evaluation results | | `classification_match` | Binary | File-based label match (observed vs expected) with `(none)`/`(other)` sentinels; emits `ClassificationCriterionResult` for suite-level P/R/F1 | diff --git a/docs/TASK_DEFINITION_GUIDE.md b/docs/TASK_DEFINITION_GUIDE.md index e96ac153..346ca59e 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -926,6 +926,7 @@ Use this instead of `command_executed` or `file_matches_regex` when a test shado log: "mocks/calls.jsonl" # Invocation log; omit it to use the record_cli default verb: "ixp projects configure-model" # Ordered prefix of the non-flag arguments positional: ["my_invoices-ixp"] # Non-flag arguments following the verb, in order + exact_positional: false # true = nothing may follow `positional` flags: model: "gemini_2_5_pro" # Bare scalar == {equals: ...} tool: "uip" # Optional: match only records with this tool @@ -934,6 +935,21 @@ Use this instead of `command_executed` or `file_matches_regex` when a test shado ignore_flags: ["output"] # Flags dropped before matching (default: ["output"]) ``` +**One operation, several verbs.** Use `verb_any_of` instead of `verb` (mutually exclusive); it matches if any entry does. Each entry is a *complete* verb in the form `verb` takes — not one token of a chain: + +```yaml +- type: "cli_called" + description: "Read the project through the CLI" + verb_any_of: ["ixp projects list", "ixp projects get"] +``` + +Do **not** shorten the verb instead. `verb: "ixp projects"` matches all 14 of its subcommands, so a positive assertion that the agent *read* a project is equally satisfied by `ixp projects delete`. Two entries are rejected when one prefixes the other, since the shorter already accepts everything the longer does. + +**Pinning the argument tail.** `positional` is a prefix too, so `verb: "ixp projects list"` also matches `ixp projects list dummy`. `exact_positional: true` requires the non-flag arguments to be exactly `positional` and nothing more; pair it with `positional: []` to assert the verb took no arguments at all. Two prerequisites: + +- **Declare value-bearing flags.** An undeclared flag is read as a switch, so its *value* stays among the positionals: `--folder Finance` turns an otherwise exactly-correct invocation into `0.0` unless `folder` appears in `value_flags` or `flags`. +- **Not on a negative guard.** Tightening suits a positive assertion. Under `max_count: 0` it works the other way — one stray argument stops the match, so the forbidden call slips past. + `log` defaults to `cli_mocks/calls.jsonl`, where [`sandbox.record_cli`](#recording-cli-invocations) writes — so a task using generated recorders never sets it. Point it elsewhere only when supplying your own mock. **Log format.** One JSON object per line. Only `argv` is required; `tool` lets one log serve several shadowed executables, and `exit`/`ts` are recorded for reporting rather than matched. Unknown keys are ignored, so a mock may record more. @@ -1021,7 +1037,7 @@ flags: **Negative guards.** Set `min_count: 0` and `max_count: 0` to assert a call did **not** happen. A missing log file *fails* rather than counting as zero matches — otherwise a mock writing to the wrong path would make every negative guard pass vacuously. -**Why not a regex over a flattened log line.** A flat `cmd arg arg` string cannot express "verb X was called AND flag Y had value Z" without stacked lookaheads; cannot distinguish a quoted argument containing spaces from two arguments; and cannot stop a match from running across shell operators. Matching `argv` element-wise removes all three problems. `verb` is an **ordered prefix**, so `ixp labellings confirm` is never satisfied by `ixp labellings unconfirm`. +**Why not a regex over a flattened log line.** A flat `cmd arg arg` string cannot express "verb X was called AND flag Y had value Z" without stacked lookaheads; cannot distinguish a quoted argument containing spaces from two arguments; and cannot stop a match from running across shell operators. Matching `argv` element-wise removes all three problems. `verb` is an **ordered prefix compared token by token**, so `ixp labellings confirm` is never satisfied by `ixp labellings unconfirm`, nor `ixp projects list` by `ixp projects lists`. What a prefix leaves open is the *tail*, which `positional` and `exact_positional` close. ### `commands_efficiency` diff --git a/src/coder_eval/criteria/cli_called.py b/src/coder_eval/criteria/cli_called.py index a7257426..e1f28785 100644 --- a/src/coder_eval/criteria/cli_called.py +++ b/src/coder_eval/criteria/cli_called.py @@ -286,10 +286,11 @@ def _check_impl( facets = [] if criterion.tool is not None: facets.append(f"tool={criterion.tool!r}") - if criterion.verb is not None: - # ' | ' rather than repr of the list: a bare list reads as "the verb is - # these tokens". A single verb renders exactly as it did before. - facets.append(f"verb={' | '.join(' '.join(t) for t in criterion.verb_spellings)!r}") + # Same source as the matcher, so the detail can never describe a different + # constraint than the one applied. Whitespace is normalized on the way through + # (`'a b'` renders as `'a b'`), which matches how the tokens were compared. + if spellings := criterion.verb_spellings: + facets.append(f"verb={' | '.join(' '.join(t) for t in spellings)!r}") if criterion.positional is not None: exact = " exactly" if criterion.exact_positional else "" facets.append(f"positional{exact}={criterion.positional!r}") diff --git a/src/coder_eval/models/criteria.py b/src/coder_eval/models/criteria.py index 58657aa8..819abb8b 100644 --- a/src/coder_eval/models/criteria.py +++ b/src/coder_eval/models/criteria.py @@ -576,17 +576,25 @@ class CliCalledCriterion(BaseSuccessCriterion): "generated recorders never repeats it" ), ) - verb: str | list[str] | None = Field( + verb: str | None = Field( default=None, description=( "Whitespace-separated subcommand chain that must be an ORDERED PREFIX of the invocation's " "non-flag arguments, compared token by token (so 'projects list' never matches " "'projects lists'). Order matters, so 'labellings confirm' never matches " - "'labellings unconfirm'. A LIST matches if ANY entry does, for a verb the tool spells " - "several ways. Prefer listing full verbs over truncating one to cover several: a short " - "verb leaves the following tokens unconstrained, which is safe for a max_count 0 guard " - "(it fires on more) but NOT for a positive assertion, where 'projects' would credit " - "'projects delete' as readily as 'projects get'" + "'labellings unconfirm'. Prefer the full verb over a short one: the tokens after it are " + "unconstrained, which is safe for a max_count 0 guard (it fires on more) but NOT for a " + "positive assertion, where 'projects' credits 'projects delete' as readily as " + "'projects get'. For one operation the tool spells several ways, use verb_any_of" + ), + ) + verb_any_of: list[str] | None = Field( + default=None, + description=( + "Alternative whole verbs; matches if ANY of them does, e.g. ['projects list', " + "'projects get']. Each entry is a complete verb in the same form `verb` takes, NOT one " + "token of a chain — a chain belongs in `verb` as a single string. Mutually exclusive " + "with `verb`" ), ) tool: str | None = Field( @@ -605,10 +613,13 @@ class CliCalledCriterion(BaseSuccessCriterion): description=( "Require the non-flag arguments after the verb to be EXACTLY `positional`, with nothing " "trailing. Without it `verb: 'projects list'` also matches `projects list dummy`. Set it " - "with `positional: []` to assert the verb took no arguments at all. Note the asymmetry " - "runs opposite to a short verb's: tightening suits a positive assertion, but on a " - "max_count 0 guard it makes the forbidden call EASIER to slip past, since one stray " - "argument stops the match" + "with `positional: []` to assert the verb took no arguments at all. Two hazards. (1) The " + "asymmetry runs opposite to a short verb's: tightening suits a positive assertion, but on " + "a max_count 0 guard it makes the forbidden call EASIER to slip past, since one stray " + "argument stops the match. (2) It depends on `value_flags` being complete: an undeclared " + "value-bearing flag leaves its VALUE among the positionals, so `--folder Finance` turns " + "an otherwise exactly-correct invocation into a 0.0 unless 'folder' is declared in " + "`value_flags` or in `flags`" ), ) flags: dict[str, FlagMatch] | None = Field( @@ -654,13 +665,52 @@ class CliCalledCriterion(BaseSuccessCriterion): def verb_spellings(self) -> list[list[str]]: """Each accepted verb as its token list; empty when there is no verb constraint. - One place splits the field, so the validator and the checker cannot disagree - about what a spelling is. + The only place either verb field is split, so the validators, the matcher and + the failure detail cannot disagree about what a spelling is. """ - if self.verb is None: - return [] - spellings = [self.verb] if isinstance(self.verb, str) else self.verb - return [spelling.split() for spelling in spellings] + if self.verb is not None: + return [self.verb.split()] + if self.verb_any_of is not None: + return [spelling.split() for spelling in self.verb_any_of] + return [] + + @model_validator(mode="after") + def _validate_verb(self) -> CliCalledCriterion: + """Verb rules, kept off _validate_bounds so neither grows unreadable.""" + if self.verb is not None and self.verb_any_of is not None: + msg = "cli_called accepts verb or verb_any_of, not both" + raise ValueError(msg) + # `verb_any_of: []` is falsy, so the "at least one facet" check would read it + # as "no verb constraint" and quietly match more than the author wrote. + if self.verb_any_of is not None and not self.verb_any_of: + msg = "cli_called verb_any_of must not be empty: drop the field to match any verb" + raise ValueError(msg) + # A character count would pass " ", and `" ".split()` is `[]` — an empty + # prefix that matches every record. + if any(not tokens for tokens in self.verb_spellings): + msg = "cli_called verb must not be blank: a blank verb is an empty prefix and matches every record" + raise ValueError(msg) + spellings = self.verb_spellings + for outer, first in enumerate(spellings): + for inner, second in enumerate(spellings): + if outer >= inner: + continue + if first == second: + msg = f"cli_called verb_any_of lists {' '.join(first)!r} twice" + raise ValueError(msg) + # A shorter entry that prefixes a longer one accepts everything the + # longer one does, so which is matched — and how many tokens it + # consumes — would depend on list order. + for shorter, longer in ((first, second), (second, first)): + if longer[: len(shorter)] == shorter: + msg = ( + f"cli_called verb_any_of entry {' '.join(shorter)!r} is a prefix of " + f"{' '.join(longer)!r}; the shorter one already accepts every invocation " + "the longer one does, so drop the longer entry or list only the verbs " + "you mean." + ) + raise ValueError(msg) + return self @model_validator(mode="after") def _validate_bounds(self) -> CliCalledCriterion: @@ -675,37 +725,6 @@ def _validate_bounds(self) -> CliCalledCriterion: if self.max_count is not None and self.max_count < self.min_count: msg = f"max_count ({self.max_count}) must be >= min_count ({self.min_count})" raise ValueError(msg) - if self.verb is not None: - spellings = [self.verb] if isinstance(self.verb, str) else self.verb - # `verb: []` is falsy, so the "at least one facet" check below would let - # it through whenever positional/flags/tool is set — as "no verb - # constraint", quietly matching more than the author wrote. - if not spellings: - msg = "cli_called verb list must not be empty: drop the field to match any verb" - raise ValueError(msg) - # A character count would pass " ", and `" ".split()` is `[]` — an - # empty prefix that matches every record. - if any(not spelling.strip() for spelling in spellings): - msg = ( - "cli_called verb must not be blank: a blank verb is an empty prefix and matches " - "every record" - ) - raise ValueError(msg) - # One candidate being a prefix of another makes the match ambiguous: both - # accept the same argv but consume a different number of tokens, so the - # offset `positional` is measured from would depend on candidate order. - # Identical entries land here too, a prefix of itself. - token_lists = [spelling.split() for spelling in spellings] - for outer, shorter in enumerate(token_lists): - for inner, longer in enumerate(token_lists): - if outer != inner and longer[: len(shorter)] == shorter: - msg = ( - f"cli_called verb {' '.join(shorter)!r} is a prefix of " - f"{' '.join(longer)!r}; both would match the same invocation while " - "consuming a different number of tokens, making the `positional` offset " - "ambiguous. List only the verbs you mean, or keep the shorter one alone." - ) - raise ValueError(msg) # `positional: []` alone asserts nothing (an empty slice equals an empty # expectation), so an author writing it to mean "took no arguments" gets a # silent no-op. exact_positional is what gives it meaning, and requiring the @@ -716,10 +735,14 @@ def _validate_bounds(self) -> CliCalledCriterion: "assert the verb took no arguments." ) raise ValueError(msg) - # Falsiness-symmetric on purpose: `verb: ""` used to slip past an `is None` - # check here and then match EVERY record (empty prefix), silently scoring 1.0. - if not self.verb and not self.positional and not self.flags and not self.tool: - msg = "cli_called requires at least one of verb / positional / flags / tool to match on" + # Falsiness on verb/flags/tool is deliberate: `verb: ""` used to slip past an + # `is None` check here and then match EVERY record (empty prefix), scoring 1.0. + # `positional` is the exception — exact_positional makes `positional: []` a real + # constraint ("zero non-flag arguments"), so falsiness there would reject an + # explicitly-set field as unset. + has_positional = self.positional is not None if self.exact_positional else bool(self.positional) + if not self.verb and not self.verb_any_of and not has_positional and not self.flags and not self.tool: + msg = "cli_called requires at least one of verb / verb_any_of / positional / flags / tool to match on" raise ValueError(msg) # A predicate on an ignored flag can never be evaluated: ignore_flags drops # the flag before any predicate runs, so `absent` would pass vacuously and diff --git a/tests/test_cli_called_criterion.py b/tests/test_cli_called_criterion.py index 7ae8e0f6..c695affb 100644 --- a/tests/test_cli_called_criterion.py +++ b/tests/test_cli_called_criterion.py @@ -759,7 +759,7 @@ def test_any_listed_spelling_matches(self, sandbox_with_log, subcommand): criterion = CliCalledCriterion( description="read the project", log=LOG, - verb=["ixp projects list", "ixp projects get"], + verb_any_of=["ixp projects list", "ixp projects get"], ) assert SuccessChecker(sandbox).check(criterion).score == 1.0 @@ -770,7 +770,7 @@ def test_an_unlisted_sibling_does_not_match(self, sandbox_with_log): criterion = CliCalledCriterion( description="read the project", log=LOG, - verb=["ixp projects list", "ixp projects get"], + verb_any_of=["ixp projects list", "ixp projects get"], ) assert SuccessChecker(sandbox).check(criterion).score == 0.0 @@ -788,7 +788,7 @@ def test_spelling_is_compared_token_by_token(self, sandbox_with_log): _call(["ixp", "projects", "list-models"]), ], ) - criterion = CliCalledCriterion(description="listed", log=LOG, verb=["ixp projects list"]) + criterion = CliCalledCriterion(description="listed", log=LOG, verb_any_of=["ixp projects list"]) assert SuccessChecker(sandbox).check(criterion).score == 0.0 @pytest.mark.parametrize("subcommand", ["publish", "unpublish"]) @@ -803,43 +803,91 @@ def test_negative_guard_fires_on_every_listed_spelling(self, sandbox_with_log, s criterion = CliCalledCriterion( description="did not change published state", log=LOG, - verb=["ixp projects publish", "ixp projects unpublish"], + verb_any_of=["ixp projects publish", "ixp projects unpublish"], min_count=0, max_count=0, ) assert SuccessChecker(sandbox).check(criterion).score == 0.0 - def test_positional_offset_follows_the_matched_spelling(self, sandbox_with_log): - """Spellings of differing length each measure `positional` from their own end.""" + @pytest.mark.parametrize( + "argv", + [ + ["ixp", "projects", "get", "proj-1"], + ["ixp", "get", "proj-1"], + ], + ids=["three-token-spelling", "two-token-spelling"], + ) + def test_positional_offset_follows_the_matched_spelling(self, sandbox_with_log, argv): + """Spellings of DIFFERING length each measure `positional` from their own end. + + Both entries must differ in length or the branch is not discriminated: with + equal-length spellings `len(matched)` equals `len(spellings[0])` and a wrong + derivation still passes. Both spellings are exercised so neither is the one + that happens to be first. + """ sandbox, sandbox_dir = sandbox_with_log - _write_log(sandbox_dir, [_call(["ixp", "fields", "delete", "proj-1"])]) + _write_log(sandbox_dir, [_call(argv)]) criterion = CliCalledCriterion( - description="deleted from the right project", + description="read the right project", log=LOG, - verb=["ixp fields remove", "ixp fields delete"], + verb_any_of=["ixp projects get", "ixp get"], positional=["proj-1"], ) assert SuccessChecker(sandbox).check(criterion).score == 1.0 + def test_wrong_positional_still_fails_under_the_shorter_spelling(self, sandbox_with_log): + """The inverse of the above: the offset must not be so large it skips the check.""" + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "get", "proj-2"])]) + criterion = CliCalledCriterion( + description="read the right project", + log=LOG, + verb_any_of=["ixp projects get", "ixp get"], + positional=["proj-1"], + ) + assert SuccessChecker(sandbox).check(criterion).score == 0.0 + + def test_score_is_independent_of_spelling_order(self, sandbox_with_log): + """Makes the "at most one can match" invariant executable. + + The prefix-collision validator exists so the offset cannot depend on order; + nothing pinned that the score doesn't either. + """ + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "get", "proj-1"])]) + forward = CliCalledCriterion( + description="d", log=LOG, verb_any_of=["ixp projects get", "ixp get"], positional=["proj-1"] + ) + reversed_ = CliCalledCriterion( + description="d", log=LOG, verb_any_of=["ixp get", "ixp projects get"], positional=["proj-1"] + ) + checker = SuccessChecker(sandbox) + assert checker.check(forward).score == checker.check(reversed_).score == 1.0 + + def test_alternation_combines_with_exact_positional(self, sandbox_with_log): + """The two features meet at the same offset arithmetic; nothing covered both.""" + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "get", "proj-1", "stray"])]) + criterion = CliCalledCriterion( + description="read exactly one project", + log=LOG, + verb_any_of=["ixp projects get", "ixp get"], + positional=["proj-1"], + exact_positional=True, + ) + assert SuccessChecker(sandbox).check(criterion).score == 0.0 + def test_failure_detail_renders_the_alternatives(self, sandbox_with_log): sandbox, sandbox_dir = sandbox_with_log _write_log(sandbox_dir, [_call(["ixp", "projects", "delete", "proj-1"])]) criterion = CliCalledCriterion( description="read the project", log=LOG, - verb=["ixp projects list", "ixp projects get"], + verb_any_of=["ixp projects list", "ixp projects get"], ) result = SuccessChecker(sandbox).check(criterion) assert "ixp projects list | ixp projects get" in (result.details or "") - def test_single_verb_detail_is_unchanged(self, sandbox_with_log): - """A plain string verb renders exactly as it did before this feature.""" - sandbox, sandbox_dir = sandbox_with_log - _write_log(sandbox_dir, [_call(["ixp", "projects", "delete", "proj-1"])]) - criterion = CliCalledCriterion(description="read", log=LOG, verb="ixp projects get") - result = SuccessChecker(sandbox).check(criterion) - assert "verb='ixp projects get'" in (result.details or "") - class TestExactPositional: """`positional` is a prefix, so trailing arguments are unconstrained by default. @@ -949,29 +997,90 @@ def test_detail_marks_the_match_as_exact(self, sandbox_with_log): def test_exact_positional_without_positional_rejected(self): """`positional: []` is the explicit way to say "no arguments".""" with pytest.raises(ValidationError, match="requires positional to be set"): - CliCalledCriterion( - description="d", log=LOG, verb="ixp projects list", exact_positional=True - ) + CliCalledCriterion(description="d", log=LOG, verb="ixp projects list", exact_positional=True) + + def test_positional_empty_with_exact_is_a_facet_on_its_own(self): + """`positional: []` + exact means "zero non-flag arguments" — a real constraint. + + The at-least-one-facet guard tests falsiness elsewhere, which would reject an + explicitly-set field as unset. + """ + criterion = CliCalledCriterion(description="d", log=LOG, positional=[], exact_positional=True) + assert criterion.positional == [] + + def test_an_undeclared_value_bearing_flag_causes_a_false_fail(self, sandbox_with_log): + """The `value_flags` coupling exact_positional introduces, pinned. + + An undeclared flag is treated as a switch, so its VALUE stays among the + positionals and the exact match rejects an invocation that was correct. The + agent ran precisely the asserted command and scores 0.0 — declaring the flag + is the fix, and this is why the field description states the prerequisite. + """ + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", "get", "proj-1", "--folder", "Finance"])]) + undeclared = CliCalledCriterion( + description="read one project", + log=LOG, + verb="ixp projects get", + positional=["proj-1"], + exact_positional=True, + ) + declared = CliCalledCriterion( + description="read one project", + log=LOG, + verb="ixp projects get", + positional=["proj-1"], + exact_positional=True, + value_flags=["output", "folder"], + ) + checker = SuccessChecker(sandbox) + assert checker.check(undeclared).score == 0.0 + assert checker.check(declared).score == 1.0 + + @pytest.mark.parametrize("verb", ["ixp projects get", "ixp projects get"]) + def test_detail_renders_the_verb_normalized(self, sandbox_with_log, verb): + """Whitespace is normalized through split()/join(), matching how it was compared.""" + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "projects", "delete", "proj-1"])]) + criterion = CliCalledCriterion(description="read", log=LOG, verb=verb) + result = SuccessChecker(sandbox).check(criterion) + assert "verb='ixp projects get'" in (result.details or "") class TestVerbAlternationValidation: + def test_a_token_chain_in_verb_is_a_type_error(self): + """The reason alternation is its own key rather than a list arm on `verb`. + + `verb: ["ixp", "projects", "list"]` is the natural way to mistype a chain. As + an alternation it would mean "ixp OR projects OR list", and the bare `ixp` + entry is a one-token prefix matching EVERY uip call — so it scored 1.0 on + `ixp projects delete`, the exact fail-open this criterion exists to prevent. + No validator can separate that from a legitimate `["list", "ls"]`, so the + schema forbids the shape instead. + """ + with pytest.raises(ValidationError, match="Input should be a valid string"): + CliCalledCriterion(description="d", log=LOG, verb=["ixp", "projects", "list"]) + + def test_verb_and_verb_any_of_together_rejected(self): + with pytest.raises(ValidationError, match="not both"): + CliCalledCriterion(description="d", log=LOG, verb="ixp projects get", verb_any_of=["ixp projects list"]) + def test_empty_list_rejected(self): - """`verb: []` is falsy, so it would slip past the at-least-one-facet check.""" + """`verb_any_of: []` is falsy, so it would slip past the at-least-one-facet check.""" with pytest.raises(ValidationError, match="must not be empty"): - CliCalledCriterion(description="d", log=LOG, verb=[], positional=["proj-1"]) + CliCalledCriterion(description="d", log=LOG, verb_any_of=[], positional=["proj-1"]) @pytest.mark.parametrize("blank", ["", " "]) def test_blank_entry_rejected(self, blank): with pytest.raises(ValidationError, match="must not be blank"): - CliCalledCriterion(description="d", log=LOG, verb=["ixp projects get", blank]) + CliCalledCriterion(description="d", log=LOG, verb_any_of=["ixp projects get", blank]) def test_spelling_that_is_a_prefix_of_another_rejected(self): - """Both match while consuming different token counts, so the `positional` - offset would depend on list order.""" + """The shorter entry already accepts everything the longer one does.""" with pytest.raises(ValidationError, match="is a prefix of"): - CliCalledCriterion(description="d", log=LOG, verb=["ixp projects", "ixp projects list"]) + CliCalledCriterion(description="d", log=LOG, verb_any_of=["ixp projects", "ixp projects list"]) - def test_duplicate_spellings_rejected(self): - """A duplicate is a prefix of itself, caught by the same rule.""" - with pytest.raises(ValidationError, match="is a prefix of"): - CliCalledCriterion(description="d", log=LOG, verb=["ixp projects get", "ixp projects get"]) + def test_duplicate_spellings_get_their_own_message(self): + """ "'a b' is a prefix of 'a b'" read as a validator bug, not a duplicate.""" + with pytest.raises(ValidationError, match="lists 'ixp projects get' twice"): + CliCalledCriterion(description="d", log=LOG, verb_any_of=["ixp projects get", "ixp projects get"]) From 277f2b504e0c0c82d9d7f717b8eacb72f9df2e80 Mon Sep 17 00:00:00 2001 From: Alexandru Jircan Date: Wed, 12 Aug 2026 12:46:11 +0300 Subject: [PATCH 04/12] revert(cli-called): drop exact_positional, keep the vacuity check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing needs it. Across the consuming IXP suite: 6 criteria are max_count 0 guards, where tightening lets the forbidden call evade; 2 set no positional, so it does not apply; the remaining 4 gain nothing, since an agent appending a stray positional to `configure-model proj-1 --model X` is not a realistic failure and the real CLI rejects it anyway. Zero of 12 would set the field. For that it charged two hazards — the negative-guard inversion, and a dependence on `value_flags` completeness that turns a correct invocation into 0.0 when an undeclared flag leaves its value among the positionals. Documenting a hazard is not the same as it being worth carrying. Review also found a bug in the field's own interaction with `positional: []`, which is the complexity cost showing up early. It was built in answer to "does `projects list dummy` still score?" — a question about the matcher's semantics, answered with a schema field before checking whether any assertion needed one. An opt-in boolean is purely additive, so adding it later breaks nothing; shipping it now makes every future reader reason about it. `verb_any_of` stays: it fixes a live hole, replacing the interim `projects delete` guard in the IXP suite that exists precisely because alternation was inexpressible. Kept from the removed work, since the trap is real without the field: `positional: []` is now REJECTED rather than silently asserting nothing (matching slices an empty expectation and compares it to itself). The at-least-one-facet check returns to plain falsiness, which is what catches `verb: ""`. Also kept: the whitespace-normalization test, which pins a rendering change to every existing single-verb config and is unrelated to the reverted field. 95 in the criterion file, 4027 in the full suite (same 8 pre-existing failures), make lint 177, ruff format/check clean. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 2 +- docs/TASK_DEFINITION_GUIDE.md | 5 +- src/coder_eval/criteria/cli_called.py | 7 +- src/coder_eval/models/criteria.py | 40 ++---- tests/test_cli_called_criterion.py | 177 ++++---------------------- 5 files changed, 37 insertions(+), 194 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ae4331f0..1f861f7a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -156,7 +156,7 @@ action.yml # Published composite GitHub Action (coder-ev | `file_matches_regex` | Binary | Regex match on file | | `reference_comparison` | Continuous | AST/token/complexity similarity | | `command_executed` | Fractional | Agent tool usage verification | -| `cli_called` | Binary | Structured match over a JSON Lines invocation log: verb (or `verb_any_of` alternation) / positional (optionally `exact_positional`) / per-flag predicates, with min_count/max_count bounds | +| `cli_called` | Binary | Structured match over a JSON Lines invocation log: verb (or `verb_any_of` alternation) / positional / per-flag predicates, with min_count/max_count bounds | | `commands_efficiency` | Continuous | Agent tool-call efficiency relative to expected budget | | `uipath_eval` | Fractional | UiPath agent evaluation results | | `classification_match` | Binary | File-based label match (observed vs expected) with `(none)`/`(other)` sentinels; emits `ClassificationCriterionResult` for suite-level P/R/F1 | diff --git a/docs/TASK_DEFINITION_GUIDE.md b/docs/TASK_DEFINITION_GUIDE.md index 346ca59e..45bbf7cd 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -926,7 +926,6 @@ Use this instead of `command_executed` or `file_matches_regex` when a test shado log: "mocks/calls.jsonl" # Invocation log; omit it to use the record_cli default verb: "ixp projects configure-model" # Ordered prefix of the non-flag arguments positional: ["my_invoices-ixp"] # Non-flag arguments following the verb, in order - exact_positional: false # true = nothing may follow `positional` flags: model: "gemini_2_5_pro" # Bare scalar == {equals: ...} tool: "uip" # Optional: match only records with this tool @@ -945,7 +944,7 @@ Use this instead of `command_executed` or `file_matches_regex` when a test shado Do **not** shorten the verb instead. `verb: "ixp projects"` matches all 14 of its subcommands, so a positive assertion that the agent *read* a project is equally satisfied by `ixp projects delete`. Two entries are rejected when one prefixes the other, since the shorter already accepts everything the longer does. -**Pinning the argument tail.** `positional` is a prefix too, so `verb: "ixp projects list"` also matches `ixp projects list dummy`. `exact_positional: true` requires the non-flag arguments to be exactly `positional` and nothing more; pair it with `positional: []` to assert the verb took no arguments at all. Two prerequisites: +**The argument tail stays open.** `positional` is a prefix too, so `verb: "ixp projects list"` with `positional: ["proj-1"]` also matches `ixp projects list proj-1 dummy`. To require a specific tail, name every argument in it. `positional: []` is rejected — it would assert nothing. - **Declare value-bearing flags.** An undeclared flag is read as a switch, so its *value* stays among the positionals: `--folder Finance` turns an otherwise exactly-correct invocation into `0.0` unless `folder` appears in `value_flags` or `flags`. - **Not on a negative guard.** Tightening suits a positive assertion. Under `max_count: 0` it works the other way — one stray argument stops the match, so the forbidden call slips past. @@ -1037,7 +1036,7 @@ flags: **Negative guards.** Set `min_count: 0` and `max_count: 0` to assert a call did **not** happen. A missing log file *fails* rather than counting as zero matches — otherwise a mock writing to the wrong path would make every negative guard pass vacuously. -**Why not a regex over a flattened log line.** A flat `cmd arg arg` string cannot express "verb X was called AND flag Y had value Z" without stacked lookaheads; cannot distinguish a quoted argument containing spaces from two arguments; and cannot stop a match from running across shell operators. Matching `argv` element-wise removes all three problems. `verb` is an **ordered prefix compared token by token**, so `ixp labellings confirm` is never satisfied by `ixp labellings unconfirm`, nor `ixp projects list` by `ixp projects lists`. What a prefix leaves open is the *tail*, which `positional` and `exact_positional` close. +**Why not a regex over a flattened log line.** A flat `cmd arg arg` string cannot express "verb X was called AND flag Y had value Z" without stacked lookaheads; cannot distinguish a quoted argument containing spaces from two arguments; and cannot stop a match from running across shell operators. Matching `argv` element-wise removes all three problems. `verb` is an **ordered prefix compared token by token**, so `ixp labellings confirm` is never satisfied by `ixp labellings unconfirm`, nor `ixp projects list` by `ixp projects lists`. What a prefix leaves open is the *tail*: `positional` constrains the arguments you name, and anything past them is unconstrained. ### `commands_efficiency` diff --git a/src/coder_eval/criteria/cli_called.py b/src/coder_eval/criteria/cli_called.py index e1f28785..3c4cbda4 100644 --- a/src/coder_eval/criteria/cli_called.py +++ b/src/coder_eval/criteria/cli_called.py @@ -167,10 +167,6 @@ def _record_matches(criterion: CliCalledCriterion, argv: list[str], record: dict expected = criterion.positional if positional[offset : offset + len(expected)] != expected: return False - # Otherwise the match is a prefix: `projects list` accepts - # `projects list dummy`, crediting a malformed invocation. - if criterion.exact_positional and len(positional) != offset + len(expected): - return False if criterion.flags: for name, predicate in criterion.flags.items(): @@ -292,8 +288,7 @@ def _check_impl( if spellings := criterion.verb_spellings: facets.append(f"verb={' | '.join(' '.join(t) for t in spellings)!r}") if criterion.positional is not None: - exact = " exactly" if criterion.exact_positional else "" - facets.append(f"positional{exact}={criterion.positional!r}") + facets.append(f"positional={criterion.positional!r}") if criterion.flags: facets.append(f"flags={sorted(criterion.flags)}") wanted = ", ".join(facets) diff --git a/src/coder_eval/models/criteria.py b/src/coder_eval/models/criteria.py index 819abb8b..e2b967a9 100644 --- a/src/coder_eval/models/criteria.py +++ b/src/coder_eval/models/criteria.py @@ -604,22 +604,9 @@ class CliCalledCriterion(BaseSuccessCriterion): positional: list[str] | None = Field( default=None, description=( - "Non-flag arguments that must follow the verb, in order. A PREFIX of what followed: " - "trailing arguments beyond these are unconstrained unless exact_positional is set" - ), - ) - exact_positional: bool = Field( - default=False, - description=( - "Require the non-flag arguments after the verb to be EXACTLY `positional`, with nothing " - "trailing. Without it `verb: 'projects list'` also matches `projects list dummy`. Set it " - "with `positional: []` to assert the verb took no arguments at all. Two hazards. (1) The " - "asymmetry runs opposite to a short verb's: tightening suits a positive assertion, but on " - "a max_count 0 guard it makes the forbidden call EASIER to slip past, since one stray " - "argument stops the match. (2) It depends on `value_flags` being complete: an undeclared " - "value-bearing flag leaves its VALUE among the positionals, so `--folder Finance` turns " - "an otherwise exactly-correct invocation into a 0.0 unless 'folder' is declared in " - "`value_flags` or in `flags`" + "Non-flag arguments that must follow the verb, in order. A PREFIX of what followed, so " + "arguments beyond these are unconstrained: 'projects list' also matches " + "'projects list dummy'. To require a specific tail, name every argument in it" ), ) flags: dict[str, FlagMatch] | None = Field( @@ -725,23 +712,18 @@ def _validate_bounds(self) -> CliCalledCriterion: if self.max_count is not None and self.max_count < self.min_count: msg = f"max_count ({self.max_count}) must be >= min_count ({self.min_count})" raise ValueError(msg) - # `positional: []` alone asserts nothing (an empty slice equals an empty - # expectation), so an author writing it to mean "took no arguments" gets a - # silent no-op. exact_positional is what gives it meaning, and requiring the - # pair keeps "exactly nothing" distinct from "unset". - if self.exact_positional and self.positional is None: + # `positional: []` asserts nothing: matching slices an empty expectation out + # of the argv and compares it to itself. An author writing it to mean "took no + # arguments" would get a silent no-op, so say so instead of accepting it. + if self.positional is not None and not self.positional: msg = ( - "cli_called exact_positional requires positional to be set. Use `positional: []` to " - "assert the verb took no arguments." + "cli_called positional must not be empty: an empty list asserts nothing. List the " + "arguments you expect, or drop the field." ) raise ValueError(msg) - # Falsiness on verb/flags/tool is deliberate: `verb: ""` used to slip past an + # Falsiness rather than `is None` on purpose: `verb: ""` used to slip past an # `is None` check here and then match EVERY record (empty prefix), scoring 1.0. - # `positional` is the exception — exact_positional makes `positional: []` a real - # constraint ("zero non-flag arguments"), so falsiness there would reject an - # explicitly-set field as unset. - has_positional = self.positional is not None if self.exact_positional else bool(self.positional) - if not self.verb and not self.verb_any_of and not has_positional and not self.flags and not self.tool: + if not self.verb and not self.verb_any_of and not self.positional and not self.flags and not self.tool: msg = "cli_called requires at least one of verb / verb_any_of / positional / flags / tool to match on" raise ValueError(msg) # A predicate on an ignored flag can never be evaluated: ignore_flags drops diff --git a/tests/test_cli_called_criterion.py b/tests/test_cli_called_criterion.py index c695affb..bfb6cde7 100644 --- a/tests/test_cli_called_criterion.py +++ b/tests/test_cli_called_criterion.py @@ -738,6 +738,15 @@ def test_criterion_with_nothing_to_match_rejected(self): with pytest.raises(ValidationError, match="at least one of"): CliCalledCriterion(description="d", log=LOG) + def test_empty_positional_rejected(self): + """`positional: []` slices an empty expectation and compares it to itself. + + It reads as "the verb took no arguments" and asserts nothing, so it is a + silent no-op — rejected rather than accepted quietly. + """ + with pytest.raises(ValidationError, match="positional must not be empty"): + CliCalledCriterion(description="d", log=LOG, verb="ixp projects list", positional=[]) + def test_unknown_field_rejected(self): with pytest.raises(ValidationError, match="Extra inputs are not permitted"): CliCalledCriterion(description="d", log=LOG, verb="v", pattern="oops") @@ -864,18 +873,21 @@ def test_score_is_independent_of_spelling_order(self, sandbox_with_log): checker = SuccessChecker(sandbox) assert checker.check(forward).score == checker.check(reversed_).score == 1.0 - def test_alternation_combines_with_exact_positional(self, sandbox_with_log): - """The two features meet at the same offset arithmetic; nothing covered both.""" + def test_trailing_arguments_stay_unconstrained(self, sandbox_with_log): + """`positional` is a prefix under alternation too, as the field says. + + Recorded so the looseness is a documented property rather than an accident: + requiring a specific tail means naming every argument in it. + """ sandbox, sandbox_dir = sandbox_with_log _write_log(sandbox_dir, [_call(["ixp", "get", "proj-1", "stray"])]) criterion = CliCalledCriterion( - description="read exactly one project", + description="read the project", log=LOG, verb_any_of=["ixp projects get", "ixp get"], positional=["proj-1"], - exact_positional=True, ) - assert SuccessChecker(sandbox).check(criterion).score == 0.0 + assert SuccessChecker(sandbox).check(criterion).score == 1.0 def test_failure_detail_renders_the_alternatives(self, sandbox_with_log): sandbox, sandbox_dir = sandbox_with_log @@ -888,158 +900,13 @@ def test_failure_detail_renders_the_alternatives(self, sandbox_with_log): result = SuccessChecker(sandbox).check(criterion) assert "ixp projects list | ixp projects get" in (result.details or "") - -class TestExactPositional: - """`positional` is a prefix, so trailing arguments are unconstrained by default. - - That credits a malformed invocation: `verb: 'projects list'` matches - `projects list dummy`, which the real CLI would reject. - """ - - def test_trailing_arguments_are_accepted_by_default(self, sandbox_with_log): - """Documents the default, so a change to it fails here rather than silently.""" - sandbox, sandbox_dir = sandbox_with_log - _write_log(sandbox_dir, [_call(["ixp", "projects", "list", "dummy"])]) - criterion = CliCalledCriterion(description="listed", log=LOG, verb="ixp projects list") - assert SuccessChecker(sandbox).check(criterion).score == 1.0 - - def test_exact_positional_rejects_trailing_arguments(self, sandbox_with_log): - sandbox, sandbox_dir = sandbox_with_log - _write_log(sandbox_dir, [_call(["ixp", "projects", "list", "dummy"])]) - criterion = CliCalledCriterion( - description="listed", - log=LOG, - verb="ixp projects list", - positional=[], - exact_positional=True, - ) - assert SuccessChecker(sandbox).check(criterion).score == 0.0 - - def test_exact_positional_accepts_the_bare_verb(self, sandbox_with_log): - """The inverse of the above: tightening must not reject the correct call.""" - sandbox, sandbox_dir = sandbox_with_log - _write_log(sandbox_dir, [_call(["ixp", "projects", "list"])]) - criterion = CliCalledCriterion( - description="listed", - log=LOG, - verb="ixp projects list", - positional=[], - exact_positional=True, - ) - assert SuccessChecker(sandbox).check(criterion).score == 1.0 - - def test_exact_positional_rejects_extra_beyond_a_listed_argument(self, sandbox_with_log): - sandbox, sandbox_dir = sandbox_with_log - _write_log(sandbox_dir, [_call(["ixp", "projects", "get", "proj-1", "proj-2"])]) - criterion = CliCalledCriterion( - description="read one project", - log=LOG, - verb="ixp projects get", - positional=["proj-1"], - exact_positional=True, - ) - assert SuccessChecker(sandbox).check(criterion).score == 0.0 - - def test_exact_positional_ignores_flags(self, sandbox_with_log): - """Only NON-flag arguments count, so `--output json` must not break it.""" - sandbox, sandbox_dir = sandbox_with_log - _write_log(sandbox_dir, [_call(["ixp", "projects", "get", "proj-1", "--output", "json"])]) - criterion = CliCalledCriterion( - description="read one project", - log=LOG, - verb="ixp projects get", - positional=["proj-1"], - exact_positional=True, - ) - assert SuccessChecker(sandbox).check(criterion).score == 1.0 - - def test_a_negative_guard_is_easier_to_evade_with_exact_positional(self, sandbox_with_log): - """The asymmetry, asserted so it is visible rather than discovered later. - - Tightening suits a positive assertion. On a max_count 0 guard it works the - other way: one stray argument stops the match, so the forbidden call slips - past. Documented on the field; pinned here. - """ - sandbox, sandbox_dir = sandbox_with_log - _write_log(sandbox_dir, [_call(["ixp", "projects", "delete", "proj-1", "stray"])]) - loose = CliCalledCriterion( - description="did not delete", - log=LOG, - verb="ixp projects delete", - min_count=0, - max_count=0, - ) - tight = CliCalledCriterion( - description="did not delete", - log=LOG, - verb="ixp projects delete", - positional=["proj-1"], - exact_positional=True, - min_count=0, - max_count=0, - ) - assert SuccessChecker(sandbox).check(loose).score == 0.0 - assert SuccessChecker(sandbox).check(tight).score == 1.0 - - def test_detail_marks_the_match_as_exact(self, sandbox_with_log): - sandbox, sandbox_dir = sandbox_with_log - _write_log(sandbox_dir, [_call(["ixp", "projects", "list", "dummy"])]) - criterion = CliCalledCriterion( - description="listed", - log=LOG, - verb="ixp projects list", - positional=[], - exact_positional=True, - ) - result = SuccessChecker(sandbox).check(criterion) - assert "positional exactly=[]" in (result.details or "") - - def test_exact_positional_without_positional_rejected(self): - """`positional: []` is the explicit way to say "no arguments".""" - with pytest.raises(ValidationError, match="requires positional to be set"): - CliCalledCriterion(description="d", log=LOG, verb="ixp projects list", exact_positional=True) - - def test_positional_empty_with_exact_is_a_facet_on_its_own(self): - """`positional: []` + exact means "zero non-flag arguments" — a real constraint. - - The at-least-one-facet guard tests falsiness elsewhere, which would reject an - explicitly-set field as unset. - """ - criterion = CliCalledCriterion(description="d", log=LOG, positional=[], exact_positional=True) - assert criterion.positional == [] - - def test_an_undeclared_value_bearing_flag_causes_a_false_fail(self, sandbox_with_log): - """The `value_flags` coupling exact_positional introduces, pinned. - - An undeclared flag is treated as a switch, so its VALUE stays among the - positionals and the exact match rejects an invocation that was correct. The - agent ran precisely the asserted command and scores 0.0 — declaring the flag - is the fix, and this is why the field description states the prerequisite. - """ - sandbox, sandbox_dir = sandbox_with_log - _write_log(sandbox_dir, [_call(["ixp", "projects", "get", "proj-1", "--folder", "Finance"])]) - undeclared = CliCalledCriterion( - description="read one project", - log=LOG, - verb="ixp projects get", - positional=["proj-1"], - exact_positional=True, - ) - declared = CliCalledCriterion( - description="read one project", - log=LOG, - verb="ixp projects get", - positional=["proj-1"], - exact_positional=True, - value_flags=["output", "folder"], - ) - checker = SuccessChecker(sandbox) - assert checker.check(undeclared).score == 0.0 - assert checker.check(declared).score == 1.0 - @pytest.mark.parametrize("verb", ["ixp projects get", "ixp projects get"]) def test_detail_renders_the_verb_normalized(self, sandbox_with_log, verb): - """Whitespace is normalized through split()/join(), matching how it was compared.""" + """The detail now round-trips through split()/join(), so whitespace normalizes. + + Deliberate — it matches how the tokens were compared — but it did change the + rendering for existing single-verb configs, so it is pinned rather than assumed. + """ sandbox, sandbox_dir = sandbox_with_log _write_log(sandbox_dir, [_call(["ixp", "projects", "delete", "proj-1"])]) criterion = CliCalledCriterion(description="read", log=LOG, verb=verb) From a6361592d03699bbd22eda2d6d3a78e81b4f6e67 Mon Sep 17 00:00:00 2001 From: Alexandru Jircan Date: Wed, 12 Aug 2026 13:59:34 +0300 Subject: [PATCH 05/12] docs(cli-called): trim comments to the whys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comment volume, not content. Cut the restatements of what the line does, the examples now carried by tests, and the sentences that paraphrased the error message two lines below. Kept the non-obvious reasons: why the offset comes from the matched spelling, why falsiness rather than `is None` in the facet check, why a character count would pass a blank verb, and why alternation is its own key. Dropped the prefix-collision comment entirely — its error message already says the same thing, better. Net -37 lines across the two files, no behavior change. 95 in the criterion file, 4027 in the full suite, make lint 177. Co-Authored-By: Claude Opus 5 (1M context) --- src/coder_eval/criteria/cli_called.py | 15 +++---- src/coder_eval/models/criteria.py | 20 ++++----- tests/test_cli_called_criterion.py | 60 +++++++-------------------- 3 files changed, 29 insertions(+), 66 deletions(-) diff --git a/src/coder_eval/criteria/cli_called.py b/src/coder_eval/criteria/cli_called.py index 3c4cbda4..101d5545 100644 --- a/src/coder_eval/criteria/cli_called.py +++ b/src/coder_eval/criteria/cli_called.py @@ -152,15 +152,13 @@ def _record_matches(criterion: CliCalledCriterion, argv: list[str], record: dict offset = 0 spellings = criterion.verb_spellings if spellings: - # ORDERED prefix compared token by token — not a subset, and not a string - # startswith: `labellings confirm` must never be satisfied by - # `labellings unconfirm`, nor `projects list` by `projects lists`. + # Token-wise, not a subset and not a string startswith: `labellings confirm` + # must never be satisfied by `labellings unconfirm`. matched = next((tokens for tokens in spellings if positional[: len(tokens)] == tokens), None) if matched is None: return False - # Offset comes from the candidate that matched, since spellings may differ in - # length. Validation rejects one spelling being a prefix of another, so at - # most one can match and this cannot depend on list order. + # Spellings may differ in length; validation rules out two matching the same + # argv, so this cannot depend on list order. offset = len(matched) if criterion.positional is not None: @@ -282,9 +280,8 @@ def _check_impl( facets = [] if criterion.tool is not None: facets.append(f"tool={criterion.tool!r}") - # Same source as the matcher, so the detail can never describe a different - # constraint than the one applied. Whitespace is normalized on the way through - # (`'a b'` renders as `'a b'`), which matches how the tokens were compared. + # Same source as the matcher, so the detail cannot describe a different + # constraint than the one applied. if spellings := criterion.verb_spellings: facets.append(f"verb={' | '.join(' '.join(t) for t in spellings)!r}") if criterion.positional is not None: diff --git a/src/coder_eval/models/criteria.py b/src/coder_eval/models/criteria.py index e2b967a9..bc11cbec 100644 --- a/src/coder_eval/models/criteria.py +++ b/src/coder_eval/models/criteria.py @@ -653,7 +653,7 @@ def verb_spellings(self) -> list[list[str]]: """Each accepted verb as its token list; empty when there is no verb constraint. The only place either verb field is split, so the validators, the matcher and - the failure detail cannot disagree about what a spelling is. + the failure detail cannot disagree. """ if self.verb is not None: return [self.verb.split()] @@ -667,13 +667,11 @@ def _validate_verb(self) -> CliCalledCriterion: if self.verb is not None and self.verb_any_of is not None: msg = "cli_called accepts verb or verb_any_of, not both" raise ValueError(msg) - # `verb_any_of: []` is falsy, so the "at least one facet" check would read it - # as "no verb constraint" and quietly match more than the author wrote. + # Falsy, so the at-least-one-facet check below would read it as "no verb". if self.verb_any_of is not None and not self.verb_any_of: msg = "cli_called verb_any_of must not be empty: drop the field to match any verb" raise ValueError(msg) - # A character count would pass " ", and `" ".split()` is `[]` — an empty - # prefix that matches every record. + # A character count would pass " ", whose split() is an empty prefix. if any(not tokens for tokens in self.verb_spellings): msg = "cli_called verb must not be blank: a blank verb is an empty prefix and matches every record" raise ValueError(msg) @@ -685,9 +683,6 @@ def _validate_verb(self) -> CliCalledCriterion: if first == second: msg = f"cli_called verb_any_of lists {' '.join(first)!r} twice" raise ValueError(msg) - # A shorter entry that prefixes a longer one accepts everything the - # longer one does, so which is matched — and how many tokens it - # consumes — would depend on list order. for shorter, longer in ((first, second), (second, first)): if longer[: len(shorter)] == shorter: msg = ( @@ -712,17 +707,16 @@ def _validate_bounds(self) -> CliCalledCriterion: if self.max_count is not None and self.max_count < self.min_count: msg = f"max_count ({self.max_count}) must be >= min_count ({self.min_count})" raise ValueError(msg) - # `positional: []` asserts nothing: matching slices an empty expectation out - # of the argv and compares it to itself. An author writing it to mean "took no - # arguments" would get a silent no-op, so say so instead of accepting it. + # Matching slices an empty expectation and compares it to itself, so this reads + # as "took no arguments" while asserting nothing. if self.positional is not None and not self.positional: msg = ( "cli_called positional must not be empty: an empty list asserts nothing. List the " "arguments you expect, or drop the field." ) raise ValueError(msg) - # Falsiness rather than `is None` on purpose: `verb: ""` used to slip past an - # `is None` check here and then match EVERY record (empty prefix), scoring 1.0. + # Falsiness, not `is None`: `verb: ""` slipped past an `is None` check here and + # then matched every record, scoring 1.0. if not self.verb and not self.verb_any_of and not self.positional and not self.flags and not self.tool: msg = "cli_called requires at least one of verb / verb_any_of / positional / flags / tool to match on" raise ValueError(msg) diff --git a/tests/test_cli_called_criterion.py b/tests/test_cli_called_criterion.py index bfb6cde7..c3ccb6ab 100644 --- a/tests/test_cli_called_criterion.py +++ b/tests/test_cli_called_criterion.py @@ -739,11 +739,7 @@ def test_criterion_with_nothing_to_match_rejected(self): CliCalledCriterion(description="d", log=LOG) def test_empty_positional_rejected(self): - """`positional: []` slices an empty expectation and compares it to itself. - - It reads as "the verb took no arguments" and asserts nothing, so it is a - silent no-op — rejected rather than accepted quietly. - """ + """Reads as "took no arguments" while asserting nothing — a silent no-op.""" with pytest.raises(ValidationError, match="positional must not be empty"): CliCalledCriterion(description="d", log=LOG, verb="ixp projects list", positional=[]) @@ -755,10 +751,8 @@ def test_unknown_field_rejected(self): class TestVerbAlternation: """A verb the tool spells several ways, e.g. the old regex's `(list|get)`. - Without alternation the only way to accept two verbs was to truncate to their - common prefix, which leaves the following tokens unconstrained — safe for a - max_count 0 guard, but on a positive assertion it credits `projects delete` as - readily as `projects get`. + The alternative was truncating to the common prefix, which on a positive assertion + credits `projects delete` as readily as `projects get`. """ @pytest.mark.parametrize("subcommand", ["list", "get"]) @@ -784,11 +778,7 @@ def test_an_unlisted_sibling_does_not_match(self, sandbox_with_log): assert SuccessChecker(sandbox).check(criterion).score == 0.0 def test_spelling_is_compared_token_by_token(self, sandbox_with_log): - """`projects list` must not match `projects lists` or `projects list-models`. - - The match is an ordered prefix over TOKENS, not a string startswith, so a - typo'd or longer-named sibling shares no token with the verb. - """ + """Prefix is over TOKENS, not a string startswith, so `lists` shares none.""" sandbox, sandbox_dir = sandbox_with_log _write_log( sandbox_dir, @@ -802,11 +792,7 @@ def test_spelling_is_compared_token_by_token(self, sandbox_with_log): @pytest.mark.parametrize("subcommand", ["publish", "unpublish"]) def test_negative_guard_fires_on_every_listed_spelling(self, sandbox_with_log, subcommand): - """The inverse: a max_count 0 guard must fail on ANY listed verb. - - A change that only widened what scores 1.0 would leave the positive tests - green while the guard quietly stopped firing. - """ + """The inverse: widening only the positive path would leave a guard silently dead.""" sandbox, sandbox_dir = sandbox_with_log _write_log(sandbox_dir, [_call(["ixp", "projects", subcommand, "proj-1"])]) criterion = CliCalledCriterion( @@ -829,10 +815,8 @@ def test_negative_guard_fires_on_every_listed_spelling(self, sandbox_with_log, s def test_positional_offset_follows_the_matched_spelling(self, sandbox_with_log, argv): """Spellings of DIFFERING length each measure `positional` from their own end. - Both entries must differ in length or the branch is not discriminated: with - equal-length spellings `len(matched)` equals `len(spellings[0])` and a wrong - derivation still passes. Both spellings are exercised so neither is the one - that happens to be first. + Equal-length spellings would not discriminate the branch: `len(matched)` would + equal `len(spellings[0])` and a wrong derivation still pass. """ sandbox, sandbox_dir = sandbox_with_log _write_log(sandbox_dir, [_call(argv)]) @@ -857,11 +841,7 @@ def test_wrong_positional_still_fails_under_the_shorter_spelling(self, sandbox_w assert SuccessChecker(sandbox).check(criterion).score == 0.0 def test_score_is_independent_of_spelling_order(self, sandbox_with_log): - """Makes the "at most one can match" invariant executable. - - The prefix-collision validator exists so the offset cannot depend on order; - nothing pinned that the score doesn't either. - """ + """The prefix-collision validator's reason for existing, made executable.""" sandbox, sandbox_dir = sandbox_with_log _write_log(sandbox_dir, [_call(["ixp", "get", "proj-1"])]) forward = CliCalledCriterion( @@ -874,11 +854,7 @@ def test_score_is_independent_of_spelling_order(self, sandbox_with_log): assert checker.check(forward).score == checker.check(reversed_).score == 1.0 def test_trailing_arguments_stay_unconstrained(self, sandbox_with_log): - """`positional` is a prefix under alternation too, as the field says. - - Recorded so the looseness is a documented property rather than an accident: - requiring a specific tail means naming every argument in it. - """ + """`positional` is a prefix under alternation too — a stated property, not an accident.""" sandbox, sandbox_dir = sandbox_with_log _write_log(sandbox_dir, [_call(["ixp", "get", "proj-1", "stray"])]) criterion = CliCalledCriterion( @@ -902,10 +878,9 @@ def test_failure_detail_renders_the_alternatives(self, sandbox_with_log): @pytest.mark.parametrize("verb", ["ixp projects get", "ixp projects get"]) def test_detail_renders_the_verb_normalized(self, sandbox_with_log, verb): - """The detail now round-trips through split()/join(), so whitespace normalizes. + """Rendering round-trips through split()/join(), so whitespace normalizes. - Deliberate — it matches how the tokens were compared — but it did change the - rendering for existing single-verb configs, so it is pinned rather than assumed. + Deliberate, but it changed the detail text for every existing single-verb config. """ sandbox, sandbox_dir = sandbox_with_log _write_log(sandbox_dir, [_call(["ixp", "projects", "delete", "proj-1"])]) @@ -916,14 +891,11 @@ def test_detail_renders_the_verb_normalized(self, sandbox_with_log, verb): class TestVerbAlternationValidation: def test_a_token_chain_in_verb_is_a_type_error(self): - """The reason alternation is its own key rather than a list arm on `verb`. - - `verb: ["ixp", "projects", "list"]` is the natural way to mistype a chain. As - an alternation it would mean "ixp OR projects OR list", and the bare `ixp` - entry is a one-token prefix matching EVERY uip call — so it scored 1.0 on - `ixp projects delete`, the exact fail-open this criterion exists to prevent. - No validator can separate that from a legitimate `["list", "ls"]`, so the - schema forbids the shape instead. + """Why alternation is its own key rather than a list arm on `verb`. + + As an alternation, the mistyped chain's bare `ixp` entry is a one-token prefix + matching every uip call — it scored 1.0 on `ixp projects delete`. Indistinguishable + from a legitimate `["list", "ls"]`, so the schema forbids the shape. """ with pytest.raises(ValidationError, match="Input should be a valid string"): CliCalledCriterion(description="d", log=LOG, verb=["ixp", "projects", "list"]) From 5ebec7fafb72b26219329197f276247e4c26afa3 Mon Sep 17 00:00:00 2001 From: Alexandru Jircan Date: Wed, 12 Aug 2026 14:04:39 +0300 Subject: [PATCH 06/12] docs(cli-called): put the offset comment's two reasons on their own lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It read "Spellings may differ in length; validation rules out two matching the same argv, so this cannot depend on list order" — two unrelated facts joined by a semicolon, with "this" pointing at nothing in particular. They explain different lines. That no argv can match two spellings is why `next()` taking the FIRST match is deterministic, so it belongs with the match. That lengths differ is why the offset comes from `matched` rather than a fixed entry. Co-Authored-By: Claude Opus 5 (1M context) --- src/coder_eval/criteria/cli_called.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/coder_eval/criteria/cli_called.py b/src/coder_eval/criteria/cli_called.py index 101d5545..06ffda9d 100644 --- a/src/coder_eval/criteria/cli_called.py +++ b/src/coder_eval/criteria/cli_called.py @@ -153,12 +153,13 @@ def _record_matches(criterion: CliCalledCriterion, argv: list[str], record: dict spellings = criterion.verb_spellings if spellings: # Token-wise, not a subset and not a string startswith: `labellings confirm` - # must never be satisfied by `labellings unconfirm`. + # must never be satisfied by `labellings unconfirm`. Taking the first match is + # safe because validation rejects one spelling prefixing another, so no argv + # can match two. matched = next((tokens for tokens in spellings if positional[: len(tokens)] == tokens), None) if matched is None: return False - # Spellings may differ in length; validation rules out two matching the same - # argv, so this cannot depend on list order. + # Measured from the spelling that matched, since spellings can differ in length. offset = len(matched) if criterion.positional is not None: From d1b59745fd8e104ac61b7f0fb61497993240f677 Mon Sep 17 00:00:00 2001 From: Alexandru Jircan Date: Wed, 12 Aug 2026 14:06:37 +0300 Subject: [PATCH 07/12] docs(cli-called): name the bug the detail renderer's source avoids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Same source as the matcher, so the detail cannot describe a different constraint than the one applied" stated a property without naming what it prevents, and left "same source" as something the reader had to work out. The concrete failure: reading `criterion.verb` here is None for a `verb_any_of` criterion, so the detail would list no verb at all — omitting the constraint that caused the failure from the message whose job is to explain it. Co-Authored-By: Claude Opus 5 (1M context) --- src/coder_eval/criteria/cli_called.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coder_eval/criteria/cli_called.py b/src/coder_eval/criteria/cli_called.py index 06ffda9d..9458ad5d 100644 --- a/src/coder_eval/criteria/cli_called.py +++ b/src/coder_eval/criteria/cli_called.py @@ -281,8 +281,8 @@ def _check_impl( facets = [] if criterion.tool is not None: facets.append(f"tool={criterion.tool!r}") - # Same source as the matcher, so the detail cannot describe a different - # constraint than the one applied. + # Reading `criterion.verb` here would print no verb at all for a `verb_any_of` + # criterion, hiding the constraint that caused the failure. if spellings := criterion.verb_spellings: facets.append(f"verb={' | '.join(' '.join(t) for t in spellings)!r}") if criterion.positional is not None: From b8fb289eed915c732a3d5f585db9ee6d3357b1b6 Mon Sep 17 00:00:00 2001 From: Alexandru Jircan Date: Wed, 12 Aug 2026 14:20:18 +0300 Subject: [PATCH 08/12] docs(cli-called): fix the guide's orphaned exact_positional prerequisites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing exact_positional left its two prerequisite bullets behind. One was dead, the other was true for the wrong reason. Dropped "not on a negative guard" — nothing tightens any more, so it described a setting that no longer exists. Kept and corrected the value_flags one. The coupling is NOT specific to exactness and survives the removal: an undeclared flag is a switch, so its value stays non-flag and takes the slot the criterion named. Verified — `get proj-1 --folder Finance` matches `positional: ["proj-1"]`, but `get --folder Finance proj-1` scores 0.0. The trigger is flag ORDER, not exactness, so the old wording ("turns an otherwise exactly-correct invocation into 0.0") pointed at the wrong cause. Now pinned by a test, and the guide states why the ambiguity resolves this way: guessing that an unknown flag consumes the next token let `--yes proj-1` bind `yes=proj-1` and swallow the project name, passing a delete guard on the delete it forbade. 96 in the criterion file, 4028 in the full suite, make lint 177. Co-Authored-By: Claude Opus 5 (1M context) --- docs/TASK_DEFINITION_GUIDE.md | 3 +-- tests/test_cli_called_criterion.py | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/docs/TASK_DEFINITION_GUIDE.md b/docs/TASK_DEFINITION_GUIDE.md index 45bbf7cd..9c287f39 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -946,8 +946,7 @@ Do **not** shorten the verb instead. `verb: "ixp projects"` matches all 14 of it **The argument tail stays open.** `positional` is a prefix too, so `verb: "ixp projects list"` with `positional: ["proj-1"]` also matches `ixp projects list proj-1 dummy`. To require a specific tail, name every argument in it. `positional: []` is rejected — it would assert nothing. -- **Declare value-bearing flags.** An undeclared flag is read as a switch, so its *value* stays among the positionals: `--folder Finance` turns an otherwise exactly-correct invocation into `0.0` unless `folder` appears in `value_flags` or `flags`. -- **Not on a negative guard.** Tightening suits a positive assertion. Under `max_count: 0` it works the other way — one stray argument stops the match, so the forbidden call slips past. +**Declare value-bearing flags when you use `positional`.** An undeclared flag is treated as a switch, so its value stays among the non-flag arguments and shifts the ones you named. `get proj-1 --folder Finance` matches `positional: ["proj-1"]`, but `get --folder Finance proj-1` does **not** — `Finance` takes the first slot. Add `folder` to `value_flags` (or name it in `flags`) to fix it. Resolving the ambiguity this way is deliberate: guessing that an unknown flag consumes the next token let `--yes proj-1` bind `yes=proj-1` and swallow the project name, which made a `max_count: 0` delete guard pass on the delete it forbade. `log` defaults to `cli_mocks/calls.jsonl`, where [`sandbox.record_cli`](#recording-cli-invocations) writes — so a task using generated recorders never sets it. Point it elsewhere only when supplying your own mock. diff --git a/tests/test_cli_called_criterion.py b/tests/test_cli_called_criterion.py index c3ccb6ab..8c2d79bb 100644 --- a/tests/test_cli_called_criterion.py +++ b/tests/test_cli_called_criterion.py @@ -743,6 +743,32 @@ def test_empty_positional_rejected(self): with pytest.raises(ValidationError, match="positional must not be empty"): CliCalledCriterion(description="d", log=LOG, verb="ixp projects list", positional=[]) + def test_an_undeclared_value_flag_before_a_positional_shifts_it(self, sandbox_with_log): + """`positional` depends on `value_flags` being complete, ordering-sensitively. + + An undeclared flag is a switch, so its value stays non-flag and takes the slot + the criterion named. Deliberate — guessing let `--yes proj-1` swallow the project + and pass a delete guard — but it costs a correct run when the flag comes first. + """ + sandbox, sandbox_dir = sandbox_with_log + _write_log( + sandbox_dir, + [_call(["ixp", "projects", "get", "--folder", "Finance", "proj-1"])], + ) + undeclared = CliCalledCriterion( + description="read the project", log=LOG, verb="ixp projects get", positional=["proj-1"] + ) + declared = CliCalledCriterion( + description="read the project", + log=LOG, + verb="ixp projects get", + positional=["proj-1"], + value_flags=["output", "folder"], + ) + checker = SuccessChecker(sandbox) + assert checker.check(undeclared).score == 0.0 + assert checker.check(declared).score == 1.0 + def test_unknown_field_rejected(self): with pytest.raises(ValidationError, match="Extra inputs are not permitted"): CliCalledCriterion(description="d", log=LOG, verb="v", pattern="oops") From a53bb461fd868353efc49d472029e015bf68eee1 Mon Sep 17 00:00:00 2001 From: Alexandru Jircan Date: Wed, 12 Aug 2026 14:34:50 +0300 Subject: [PATCH 09/12] docs(cli-called): fix two unclear field descriptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `verb`: "For one operation the tool spells several ways, use verb_any_of" is a garden-path sentence. `positional`: it called itself a prefix and then illustrated the point with 'projects list' vs 'projects list dummy' — an example containing no positional at all, so it demonstrated verb-prefix looseness on a field about arguments. Uses a real positional now, and names the value_flags dependency that shifts these slots. Co-Authored-By: Claude Opus 5 (1M context) --- src/coder_eval/models/criteria.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/coder_eval/models/criteria.py b/src/coder_eval/models/criteria.py index bc11cbec..3b36fc92 100644 --- a/src/coder_eval/models/criteria.py +++ b/src/coder_eval/models/criteria.py @@ -585,7 +585,7 @@ class CliCalledCriterion(BaseSuccessCriterion): "'labellings unconfirm'. Prefer the full verb over a short one: the tokens after it are " "unconstrained, which is safe for a max_count 0 guard (it fires on more) but NOT for a " "positive assertion, where 'projects' credits 'projects delete' as readily as " - "'projects get'. For one operation the tool spells several ways, use verb_any_of" + "'projects get'. When one operation has several spellings, use verb_any_of" ), ) verb_any_of: list[str] | None = Field( @@ -605,8 +605,9 @@ class CliCalledCriterion(BaseSuccessCriterion): default=None, description=( "Non-flag arguments that must follow the verb, in order. A PREFIX of what followed, so " - "arguments beyond these are unconstrained: 'projects list' also matches " - "'projects list dummy'. To require a specific tail, name every argument in it" + "anything past them is unconstrained: ['proj-1'] also matches 'get proj-1 dummy'. To " + "require a specific tail, name every argument in it. Depends on value_flags being " + "complete — an undeclared flag's value stays non-flag and shifts these slots" ), ) flags: dict[str, FlagMatch] | None = Field( From 653f94a93bf12343e5e3c8502f22fd049ff2e356 Mon Sep 17 00:00:00 2001 From: Alexandru Jircan Date: Wed, 12 Aug 2026 16:02:32 +0300 Subject: [PATCH 10/12] docs(cli-called): drop the hardcoded subcommand count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review suggestion from @cezara98t. "all 14 of its subcommands" is a point-in-time fact about one CLI version — the catalog is refreshed by a bot, so the number rots while the sentence still reads as authoritative. The argument does not need it. Co-Authored-By: Claude Opus 5 (1M context) --- docs/TASK_DEFINITION_GUIDE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/TASK_DEFINITION_GUIDE.md b/docs/TASK_DEFINITION_GUIDE.md index 9c287f39..4bf8a40c 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -942,7 +942,7 @@ Use this instead of `command_executed` or `file_matches_regex` when a test shado verb_any_of: ["ixp projects list", "ixp projects get"] ``` -Do **not** shorten the verb instead. `verb: "ixp projects"` matches all 14 of its subcommands, so a positive assertion that the agent *read* a project is equally satisfied by `ixp projects delete`. Two entries are rejected when one prefixes the other, since the shorter already accepts everything the longer does. +Do **not** shorten the verb instead. `verb: "ixp projects"` matches all of its subcommands, so a positive assertion that the agent *read* a project is equally satisfied by `ixp projects delete`. Two entries are rejected when one prefixes the other, since the shorter already accepts everything the longer does. **The argument tail stays open.** `positional` is a prefix too, so `verb: "ixp projects list"` with `positional: ["proj-1"]` also matches `ixp projects list proj-1 dummy`. To require a specific tail, name every argument in it. `positional: []` is rejected — it would assert nothing. From 3460349a20093456ebcc7fffe6062128371c4993 Mon Sep 17 00:00:00 2001 From: Alexandru Jircan Date: Tue, 18 Aug 2026 09:47:08 +0300 Subject: [PATCH 11/12] chore(plugin): regenerate the bundled criteria reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CE033 blocker from @uipreliga's review. `plugins/coder-eval/reference/criteria.md` is generated from the SuccessCriterion union, so adding verb_any_of and rewriting the verb/positional descriptions left the bundled copy stale — an installed plugin would have shipped users the one surface saying the feature does not exist. I deferred this earlier on the grounds that `make plugin-reference` lived on feat/claude-code-plugin. That was true when I checked and is not now: #82 merged the plugin into main, so the target and the parity gate are both here. Reproduced the failure first (test_generated_reference_matches_disk), regenerated, and confirmed it passes. The remaining CE033 failure locally, test_drift_is_detected, is a Windows-only pre-existing bug — the test writes with the platform cp1252 default and reads back as UTF-8; it fails identically on a clean main worktree at ea5a3fc. Co-Authored-By: Claude Opus 5 (1M context) --- plugins/coder-eval/reference/criteria.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/plugins/coder-eval/reference/criteria.md b/plugins/coder-eval/reference/criteria.md index ac7d2b0f..57fe03f8 100644 --- a/plugins/coder-eval/reference/criteria.md +++ b/plugins/coder-eval/reference/criteria.md @@ -87,9 +87,10 @@ Optional: | Field | What it is | | --- | --- | | `log` | Path to the JSON Lines invocation log, relative to the sandbox working directory. Defaults to 'cli_mocks/calls.jsonl', where SandboxConfig.record_cli writes, so a task using generated recorders never repeats it | -| `verb` | Whitespace-separated subcommand chain that must be an ORDERED PREFIX of the invocation's non-flag arguments. Order matters, so 'labellings confirm' never matches 'labellings unconfirm' | +| `verb` | Whitespace-separated subcommand chain that must be an ORDERED PREFIX of the invocation's non-flag arguments, compared token by token (so 'projects list' never matches 'projects lists'). Order matters, so 'labellings confirm' never matches 'labellings unconfirm'. Prefer the full verb over a short one: the tokens after it are unconstrained, which is safe for a max_count 0 guard (it fires on more) but NOT for a positive assertion, where 'projects' credits 'projects delete' as readily as 'projects get'. When one operation has several spellings, use verb_any_of | +| `verb_any_of` | Alternative whole verbs; matches if ANY of them does, e.g. ['projects list', 'projects get']. Each entry is a complete verb in the same form `verb` takes, NOT one token of a chain — a chain belongs in `verb` as a single string. Mutually exclusive with `verb` | | `tool` | Match only records whose 'tool' equals this (e.g. 'uip'). None matches any tool | -| `positional` | Non-flag arguments that must follow the verb, in order | +| `positional` | Non-flag arguments that must follow the verb, in order. A PREFIX of what followed, so anything past them is unconstrained: ['proj-1'] also matches 'get proj-1 dummy'. To require a specific tail, name every argument in it. Depends on value_flags being complete — an undeclared flag's value stays non-flag and shifts these slots | | `flags` | Flag name (without leading dashes) to predicate. A bare scalar means 'equals'. Flags not listed here are ignored, so an extra --output json never breaks a match | | `value_flags` | Flag names (no leading dashes) that consume a following token as their value. Keys of `flags` are value-bearing already; everything else is a switch whose following token stays positional. Declare a flag here when its value would otherwise be read as a positional, e.g. [folder] for `--folder F proj-1`. Defaults to [output] | | `min_count` | Minimum matching invocations. Combine min_count: 0 with max_count: 0 for must-NOT-match. Scoring is BINARY (in vs out of bounds), unlike command_executed's fractional field of the same name | From bfb28d384a62085ed7415d6916e8228f522e581a Mon Sep 17 00:00:00 2001 From: Alexandru Jircan Date: Tue, 18 Aug 2026 09:47:17 +0300 Subject: [PATCH 12/12] refactor(cli-called): collapse the pairwise verb check to itertools.combinations Optional suggestion from @uipreliga, applied as given. The nested enumerate with an `outer >= inner` skip plus a both-directions inner tuple was hand-rolling combinations; -6 lines. Recorded why the length sort is safe, since that is the step that replaces checking both directions: two DISTINCT entries of equal length cannot prefix each other, because an equal-length prefix is the same list. Co-Authored-By: Claude Opus 5 (1M context) --- src/coder_eval/models/criteria.py | 32 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/coder_eval/models/criteria.py b/src/coder_eval/models/criteria.py index 3a23d9f5..aa584168 100644 --- a/src/coder_eval/models/criteria.py +++ b/src/coder_eval/models/criteria.py @@ -8,6 +8,7 @@ from __future__ import annotations +import itertools from abc import ABC, abstractmethod from typing import Annotated, Any, ClassVar, Literal, Self @@ -676,23 +677,20 @@ def _validate_verb(self) -> CliCalledCriterion: if any(not tokens for tokens in self.verb_spellings): msg = "cli_called verb must not be blank: a blank verb is an empty prefix and matches every record" raise ValueError(msg) - spellings = self.verb_spellings - for outer, first in enumerate(spellings): - for inner, second in enumerate(spellings): - if outer >= inner: - continue - if first == second: - msg = f"cli_called verb_any_of lists {' '.join(first)!r} twice" - raise ValueError(msg) - for shorter, longer in ((first, second), (second, first)): - if longer[: len(shorter)] == shorter: - msg = ( - f"cli_called verb_any_of entry {' '.join(shorter)!r} is a prefix of " - f"{' '.join(longer)!r}; the shorter one already accepts every invocation " - "the longer one does, so drop the longer entry or list only the verbs " - "you mean." - ) - raise ValueError(msg) + for first, second in itertools.combinations(self.verb_spellings, 2): + if first == second: + msg = f"cli_called verb_any_of lists {' '.join(first)!r} twice" + raise ValueError(msg) + # Sorting by length is total here: two DISTINCT entries of equal length + # cannot prefix each other, since an equal-length prefix is the same list. + shorter, longer = sorted((first, second), key=len) + if longer[: len(shorter)] == shorter: + msg = ( + f"cli_called verb_any_of entry {' '.join(shorter)!r} is a prefix of " + f"{' '.join(longer)!r}; the shorter one already accepts every invocation the " + "longer one does, so drop the longer entry or list only the verbs you mean." + ) + raise ValueError(msg) return self @model_validator(mode="after")