diff --git a/CLAUDE.md b/CLAUDE.md index a1402582..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 / 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 e96ac153..4bf8a40c 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -934,6 +934,20 @@ 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 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. + +**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. **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 +1035,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*: `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 74532c68..9458ad5d 100644 --- a/src/coder_eval/criteria/cli_called.py +++ b/src/coder_eval/criteria/cli_called.py @@ -150,14 +150,17 @@ 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: + # Token-wise, not a subset and not a string startswith: `labellings confirm` + # 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 - offset = len(verb_tokens) + # Measured from the spelling that matched, since spellings can differ in length. + offset = len(matched) if criterion.positional is not None: expected = criterion.positional @@ -278,8 +281,10 @@ def _check_impl( facets = [] 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}") + # 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: 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..3b36fc92 100644 --- a/src/coder_eval/models/criteria.py +++ b/src/coder_eval/models/criteria.py @@ -578,11 +578,23 @@ class CliCalledCriterion(BaseSuccessCriterion): ) verb: 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'. 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: 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( @@ -591,7 +603,12 @@ 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, 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: dict[str, FlagMatch] | None = Field( default=None, @@ -632,6 +649,52 @@ 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. + + The only place either verb field is split, so the validators, the matcher and + the failure detail cannot disagree. + """ + 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) + # 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 " ", 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) + 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) + return self + @model_validator(mode="after") def _validate_bounds(self) -> CliCalledCriterion: # min_count 0 with no upper bound is satisfied by every possible log, so @@ -645,15 +708,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) - # 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" + # 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-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, 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) # 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 bda64b92..8c2d79bb 100644 --- a/tests/test_cli_called_criterion.py +++ b/tests/test_cli_called_criterion.py @@ -738,6 +738,214 @@ 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): + """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=[]) + + 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") + + +class TestVerbAlternation: + """A verb the tool spells several ways, e.g. the old regex's `(list|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"]) + 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_any_of=["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_any_of=["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): + """Prefix is over TOKENS, not a string startswith, so `lists` shares none.""" + 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_any_of=["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: 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( + description="did not change published state", + log=LOG, + verb_any_of=["ixp projects publish", "ixp projects unpublish"], + min_count=0, + max_count=0, + ) + assert SuccessChecker(sandbox).check(criterion).score == 0.0 + + @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. + + 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)]) + 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 == 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): + """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( + 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_trailing_arguments_stay_unconstrained(self, sandbox_with_log): + """`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( + description="read the project", + log=LOG, + verb_any_of=["ixp projects get", "ixp get"], + 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_any_of=["ixp projects list", "ixp projects get"], + ) + result = SuccessChecker(sandbox).check(criterion) + assert "ixp projects list | ixp projects get" in (result.details or "") + + @pytest.mark.parametrize("verb", ["ixp projects get", "ixp projects get"]) + def test_detail_renders_the_verb_normalized(self, sandbox_with_log, verb): + """Rendering round-trips through split()/join(), so whitespace normalizes. + + 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"])]) + 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): + """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"]) + + 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_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_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_any_of=["ixp projects get", blank]) + + def test_spelling_that_is_a_prefix_of_another_rejected(self): + """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_any_of=["ixp projects", "ixp projects list"]) + + 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"])