Skip to content

feat(cli-called): accept a list of verb spellings - #103

Open
alexandrujircan wants to merge 2 commits into
mainfrom
feat/cli-called-verb-alternation
Open

feat(cli-called): accept a list of verb spellings#103
alexandrujircan wants to merge 2 commits into
mainfrom
feat/cli-called-verb-alternation

Conversation

@alexandrujircan

Copy link
Copy Markdown
Contributor

cli_called.verb now accepts a list of spellings, matching if any entry does.

The problem

verb was a single string matched as an ordered prefix, so a criterion needing "list OR get" had exactly one option: truncate to the common prefix. That leaves every following token unconstrained — which is safe for a max_count: 0 guard (it fires on more) but wrong for a positive assertion.

Found in a real task. A weight-3.0 criterion asserting the agent read a project, written as verb: "ixp projects":

agent ran prefix (today) alternation
projects get 1.0 1.0
projects list 1.0 1.0
projects delete 1.0 0.0
projects update-title 1.0 0.0
projects publish 1.0 0.0
projects fetch-meta (hallucinated) 1.0 0.0

The regex it replaced said (list|get) and admitted none of those. So the migration to structured matching was a regression, and the API is why: the unsafe option was the only expressible one.

The change

verb: ["ixp projects list", "ixp projects get"]

The docstring now states the breadth asymmetry it left implicit — order was documented ("labellings confirm never matches labellings unconfirm"), widening was not.

Note "exact verb matching" is deliberately not what this adds: cli_called has no CLI grammar, so ["ixp","projects","get","proj-1"] is the same argv whether get is a verb token or a positional. The author supplies the boundary — a fuller verb, or positional. This makes the safe boundary expressible when the tool spells one operation several ways.

Validation added

Each of these either matches every record or resolves 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 matching everything
  • one spelling a prefix of another rejected — both match the same argv while consuming different token counts, so the positional offset would depend on list order. Catches duplicates too, being prefixes of themselves.

Tests

27 new, including:

  • the inverse — a max_count: 0 guard must fire on every listed spelling. A change that only widened the positive path would leave the positive tests green while the guard quietly stopped firing.
  • token-by-token comparisonprojects list must not match projects lists or projects list-models. Previously untested, and it's the property that makes "list full verbs" a sufficient answer rather than a partial one.
  • positional offset following the matched spelling when lengths differ.
  • single-string detail rendering byte-identical; lists render a | b rather than a bare repr, which would read as "the verb is these tokens".

Gate

  • tests/test_cli_called_criterion.py: 87 passed
  • Full suite: 4019 passed, 8 failed — all 8 reproduce identically on clean main (3 × reports_stats_nonfinite float/numerator, 2 × test_sandbox Windows symlink privilege, 3 × claude_settings_enforcement_live). Verified by running them in a worktree at 57556af.
  • make lint: 177 passed

Backwards compatible: verb as a string behaves exactly as before, including its failure-detail text.

🤖 Generated with Claude Code

`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) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown

Claude finished @alexandrujircan's task in 1m 27s —— View job


Code Review In Progress

Todo List:

  • Read review guidelines in .github/code_review.md
  • Read CLAUDE.md for project conventions
  • Review the full diff with git diff origin/main...HEAD
  • Read complete changed files for context
  • Check for cross-file consistency
  • Perform design-level analysis
  • Check for missing elements
  • Post comprehensive review feedback

Starting review...

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) <noreply@anthropic.com>

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Scope: pr:103 (3 files) axis:1,2,3,4,5,6,7,8 · branch feat/cli-called-verb-alternation · fea3089 · 2026-08-11T15:43Z · workflow variant

Change class: complex — changes the matching semantics and schema of a scoring criterion (verb alternation + exact_positional), plus new validator branches; correctness requires reasoning about how each shape scores an argv

coder_eval remains in strong shape at 9.4/10 — security and error handling are clean, the architecture and harness invariants hold, and no critical or blocking defects exist — but this PR's cli_called extensions ship two scoring hazards (a list-valued verb that silently over-matches forbidden invocations, and exact_positional turning any undeclared value-bearing flag into a false FAIL), a test that doesn't discriminate the branch it claims to guard, and a red ruff format --check gate, so the bottom line is: fix the two verdict-affecting schema hazards and the format gate before merge, then close the doc and validator-complexity debt.

Summary

Axis Score 🔴 🟠 🟡 🔵 Top Issue
1. Code Quality & Style 8.9 / 10 0 0 2 1 🟡 ruff format --check fails on two files this PR touches — make verify and the CI format gate (pr-checks.yml, Ubuntu + Windows) go red
2. Type Safety 8.9 / 10 0 1 0 1 `verb: str
3. Test Health 8.9 / 10 0 1 0 1 The only test for the new offset-from-matched-spelling branch uses two equal-length spellings, so the branch is never discriminated (mutation survives the whole 95-test suite)
4. Security 10 / 10 0 0 0 0
5. Architecture & Design 9.5 / 10 0 0 1 0 verb scalar-or-list normalization + .split() are duplicated in _validate_bounds, contradicting verb_spellings' "one place splits the field" docstring
6. Error Handling & Resilience 10 / 10 0 0 0 0
7. API Surface & Maintainability 9.5 / 10 0 0 1 0 New exact_positional field and the list form of verb ship undocumented (TASK_DEFINITION_GUIDE.md and PR description)
8. Evaluation Harness Quality 9.5 / 10 0 0 1 0 exact_positional makes the verdict depend on value_flags completeness — an undeclared value-bearing flag turns a correct invocation into a false FAIL (undocumented, untested)

Overall Score: 9.4 / 10 · Weakest Axis: Code Quality & Style at 8.9 / 10
Totals: 🔴 0 · 🟠 2 · 🟡 5 · 🔵 3 across 8 axes.

Blockers

  1. [Axis 2] verb: str | list[str] list arm means alternation while the sibling positional: list[str] means an ordered token chain — a token chain written as a list validates and silently over-matches (src/coder_eval/models/criteria.py:579) — The field widens to verb: str | list[str] | None = Field( (line 579) where the STRING arm is a whitespace-separated token chain but the LIST arm is a set of alternative whole chains. Those two readings collide on the most natural conversion a task author will make. Verified by execution against the PR HEAD:
>>> c = CliCalledCriterion(description='d', verb=['ixp','projects','list'], min_count=1)
>>> c.verb_spellings
[['ixp'], ['projects'], ['list']]
>>> _record_matches(c, ['ixp','projects','delete','proj-1','--yes'], {'tool':'ixp'})
True      # <-- scores 1.0 on the DELETE the author never asked for
>>> _record_matches(CliCalledCriterion(description='d', verb='ixp projects list'), same_argv, ...)
False     # the string form is correct

Nothing rejects it: the new pairwise guard at models/criteria.py:701 only fires when longer[: len(shorter)] == shorter, and ['ixp'] / ['projects'] / ['list'] are pairwise non-prefixes, so _validate_bounds passes. The result is a well-formed task YAML that silently grades a forbidden invocation as a pass — exactly the vacuity class the surrounding validators (blank verb, min_count: 0 + no max_count) were written to close, and it is unreachable by the existing checks because a single-token alternation like ['list','ls'] is legitimate. This is not statically separable from the legitimate case, so the fix has to be at the schema, not in a validator: either give alternation its own key (e.g. verb_any_of: list[str], leaving verb a plain str), or require the list arm's entries to be tagged/multi-token. At minimum add the confusable case to tests/test_cli_called_criterion.py::TestVerbAlternationValidation, which today has no test for it.
2. [Axis 3] The only test for the new offset-from-matched-spelling branch uses two equal-length spellings, so the branch is never discriminated (mutation survives the whole 95-test suite) (tests/test_cli_called_criterion.py:813) — _record_matches derives the positional offset from the spelling that actually matched — src/coder_eval/criteria/cli_called.py:164 offset = len(matched) — justified by the comment at 161-163 ("Offset comes from the candidate that matched, since spellings may differ in length"). The single test guarding this, at tests/test_cli_called_criterion.py:812-822, claims exactly that contract in its docstring at line 813 — """Spellings of differing length each measure positional from their own end.""" — but the spellings it passes at line 819, verb=["ixp fields remove", "ixp fields delete"], are BOTH 3 tokens long, so len(matched) and len(spellings[0]) are identical and the branch is not exercised. No other test in the file uses differing-length spellings (the other alternation tests use ["ixp projects list", "ixp projects get"] and ["ixp projects publish", "ixp projects unpublish"], also equal-length). Proven by mutation: replacing line 164 with offset = len(spellings[0]) and re-running uv run pytest tests/test_cli_called_criterion.py still reports 95 passed. This is the score-changing gate the axis brief asks to have tripped: with the wrong derivation, verb=["ixp projects get", "ixp get"] + positional=["proj-1"] would score 0.0 on the correct invocation ixp get proj-1 (the real code returns True for both ['ixp','get','proj-1'] and ['ixp','projects','get','proj-1']). Fix: change line 819 to genuinely differing-length spellings (e.g. verb=["ixp projects get", "ixp get"]) and add the mirrored case that matches the OTHER spelling, so the mutation fails. While there, add the missing cross-feature case — no test anywhere combines a list verb with exact_positional=True, even though both features meet at this same offset.

Non-blocking, but please consider before merge

  1. [Axis 1] 🟡 ruff format --check fails on two files this PR touches — make verify and the CI format gate (pr-checks.yml, Ubuntu + Windows) go red (src/coder_eval/models/criteria.py:689) — Verified with uv run ruff format --check src/ tests/ (the exact LINT_PATHS at Makefile:22, used by verify: at Makefile:51): 2 files would be reformatted, both introduced by this PR. main's copies of both are clean.

(a) src/coder_eval/models/criteria.py:689-692 — the implicit-concat wrap is under ruff's 120-char limit as a single line:

                msg = (
                    "cli_called verb must not be blank: a blank verb is an empty prefix and matches "
                    "every record"
                )

ruff wants: msg = "cli_called verb must not be blank: a blank verb is an empty prefix and matches every record"

(b) tests/test_cli_called_criterion.py:949-953 — the routed signal only covered src/, but make verify lints tests/ too:

        with pytest.raises(ValidationError, match="requires positional to be set"):
            CliCalledCriterion(
                description="d", log=LOG, verb="ixp projects list", exact_positional=True
            )

ruff wants the call collapsed onto one line.

Fix: run make format and commit the result. No behavioral change.
2. [Axis 1] CliCalledCriterion._validate_bounds complexity grows to CC 29 (C->D) owning many unrelated validation concerns (src/coder_eval/models/criteria.py:666) — Measured with uv run radon cc -s, PR HEAD (fea3089) vs origin/main (same files extracted via git show origin/main:<path>):

block main PR
CliCalledCriterion._validate_bounds (models/criteria.py:666) C (19) D (29)
_record_matches (criteria/cli_called.py:135) C (20) D (24)
CliCalledChecker._check_impl (criteria/cli_called.py:192) D (24) D (26)

Repo average is B (5.46) over 1076 blocks. _validate_bounds is now a single 53-line method (lines 666-718 plus the flag-alias block below it) enforcing seven unrelated rules: count vacuity, count ordering, empty verb list, blank verb entry, the O(n²) pairwise-prefix scan (699-708), exact_positional/positional pairing, and the at-least-one-facet check. Only the last is shared state; the rest are independent.

Recommendation: split the verb rules out into their own @model_validator(mode="after") (e.g. _validate_verb) — pydantic runs after-validators in declaration order, so behavior and message ordering are preserved, and each validator lands back in the A/B band. Filed 🟡 rather than 🟠 because models/criteria.py is not one of the anchor table's named hot modules (orchestrator, checker, sandbox), and per the Severity Standard's tie-break I take the lower level.
3. [Axis 5] verb scalar-or-list normalization + .split() are duplicated in _validate_bounds, contradicting verb_spellings' "one place splits the field" docstring (src/coder_eval/models/criteria.py:662) — CliCalledCriterion is the ONLY criterion model in the file that declares a scalar-or-list union — every other "one or more" field is a plain list (FlagMatch.any_of L441, FlagMatch.aliases L458, allowed_labels L1120), and the one field in this file that accepts a scalar shorthand normalizes it ONCE in a before-validator instead of widening the declared type:

L490-L496  @model_validator(mode="before")
           @classmethod
           def _coerce_scalar_shorthand(cls, value: Any) -> Any:
               """Accept ``model: gemini_2_5_pro`` as ``model: {equals: ...}``."""
               if isinstance(value, str):
                   return {"equals": value}
               return value

This PR takes the other route, and the union then has to be un-widened at every read site. The exact same normalization expression appears twice, 17 lines apart:

L662 (in `verb_spellings`)   spellings = [self.verb] if isinstance(self.verb, str) else self.verb
L679 (in `_validate_bounds`) spellings = [self.verb] if isinstance(self.verb, str) else self.verb

and the split it wraps is duplicated too — return [spelling.split() for spelling in spellings] (L663) vs token_lists = [spelling.split() for spelling in spellings] (L698). That directly contradicts the new property's own docstring at L657-658: "One place splits the field, so the validator and the checker cannot disagree about what a spelling is." The validator never calls verb_spellings; it re-derives it. The union also leaks into the checker as two different spellings of "is there a verb constraint": if spellings: at criteria/cli_called.py:154 versus if criterion.verb is not None: at criteria/cli_called.py:289.

Fix: declare verb: list[str] | None and add a mode="before" model validator on CliCalledCriterion that wraps a bare string ({"verb": "a b"} -> {"verb": ["a b"]}), mirroring FlagMatch._coerce_scalar_shorthand. YAML authoring stays backward compatible, the declared type stays single-shaped, verb_spellings loses its isinstance branch, and _validate_bounds reads self.verb directly. At minimum, have _validate_bounds call self.verb_spellings for token_lists instead of re-splitting at L698, so the property's stated single-source claim is true.
4. [Axis 7] New exact_positional field and the list form of verb ship undocumented (TASK_DEFINITION_GUIDE.md and PR description) (src/coder_eval/models/criteria.py:603) — docs/TASK_DEFINITION_GUIDE.md § ### \cli_called`(starts line 917) is the only user-facing reference for this schema, and the PR does not touch it. Its example still readsverb: "ixp projects configure-model" # Ordered prefix of the non-flag arguments(line 927) andpositional: ["my_invoices-ixp"] # Non-flag arguments following the verb, in order (line 928); the closing rationale at line 1024 repeats "verbis an **ordered prefix**" with no mention of alternation.exact_positionalappears nowhere outsidesrc/andtests/test_cli_called_criterion.py(verified:grep -rn exact_positional --include=*.md .returns nothing). No lint catches this —tests/lint/doc_schema_parity.py(CE030) registers onlyTaskDefinition, RunLimits, Dataset, SimulationConfigand explicitly states "nested models (``AgentConfig``, ``SandboxConfig``, criteria, …) are NOT walked"; I ranmake lintin the worktree and got177 passed, so nothing fires. Add to the guide's cli_calledsection: theverb: [a, b]alternation form (with the prefix-collision rejection rule) and anexact_positionalrow/paragraph, and consider extending the CE030 registry (or a sibling rule) to cover the criterion models so the next added field cannot ship undocumented. 5. **[Axis 8]exact_positionalmakes the verdict depend onvalue_flags completeness — an undeclared value-bearing flag turns a correct invocation into a false FAIL (undocumented, untested)** (src/coder_eval/criteria/cli_called.py:172) — Verified at the PR HEAD: CliCalledCriterion(description='d', verb='ixp projects list', positional=['proj-1'], exact_positional=True, min_count=1)gives_record_matches(..., ['ixp','projects','list','proj-1','--folder','Finance'], {}) -> False. --folderis not in the defaultvalue_flags (['output'], models/criteria.py:621-629), so _split_flagscorrectly leavesFinanceinpositional; the new line if criterion.exact_positional and len(positional) != offset + len(expected): return Falsethen rejects the match. The agent ran exactly the asserted command and the criterion scores 0.0. Withoutexact_positionalthis same undeclared-value-flag hazard was bounded — the stray token only mattered if it landed inside thepositional[offset:offset+len(expected)]slice — so the new field materially amplifies it: with it, ANY undeclared value flag anywhere after the verb breaks the match.docs/TASK_DEFINITION_GUIDE.md:970-979documents thevalue_flagsrequirement generally, but the new field'sdescription= (models/criteria.py:605-612) discusses only the max_count: 0asymmetry and never mentions thatexact_positionalrequires every value-bearing flag the CLI may emit to be declared. Fix: state thevalue_flagsprerequisite in theexact_positionaldescription and in the guide, and add a test asserting the false-FAIL shape above so the coupling is visible (the addedTestExactPositional::test_exact_positional_ignores_flags covers only the already-declared/--output` case, which is the benign direction).

Nits

  1. [Axis 1] New verb-list validator messages misdescribe the duplicate-entry case and cite a positional offset/entry that may not exist (src/coder_eval/models/criteria.py:702) — Two message-accuracy problems in the pairwise scan at models/criteria.py:699-708:

(1) Duplicates. The comment at line 697 acknowledges # Identical entries land here too, a prefix of itself. — but the message was written for the strictly-shorter case. Verified by running the model:

$ CliCalledCriterion(description='d', log='l.jsonl', verb=['a b', 'a b'])
Value error, cli_called verb 'a b' is a prefix of 'a b'; ... List only the verbs you mean, or keep the shorter one alone.

There is no "shorter one", and "'a b' is a prefix of 'a b'" reads as a bug in the validator rather than as a duplicate entry. tests/test_cli_called_criterion.py:983 (test_duplicate_spellings_rejected) only asserts match="is a prefix of", so it passes on the confusing text.

(2) The justification names a field the author may not have set. Lines 705-706 say the collision makes "the positional offset ambiguous", but in criteria/cli_called.py the offset computed at line 162 (offset = len(matched)) is read only inside if criterion.positional is not None: (line 167) — with no positional, offset is dead and the collision is harmless. So verb: ["ixp projects", "ixp projects list"] with no positional is rejected with a rationale that does not apply to it.

Fix: add a shorter == longer branch emitting a dedicated "duplicate verb spelling" message, and either scope the check to self.positional is not None or reword lines 705-706 to state the rule without asserting a positional the config may lack (e.g. "…would consume a different number of tokens, so which one is matched would depend on list order").
2. [Axis 2] not self.positional conflates positional: [] with unset, so positional: [] + exact_positional is rejected by an error claiming positional is unset (src/coder_eval/models/criteria.py:721) — exact_positional makes positional: [] a real constraint for the first time (its own description says "Set it with positional: [] to assert the verb took no arguments at all", line 608), but the at-least-one-facet guard still tests falsiness rather than is None:

721:        if not self.verb and not self.positional and not self.flags and not self.tool:
722:            msg = "cli_called requires at least one of verb / positional / flags / tool to match on"

Verified against the PR HEAD: CliCalledCriterion(description='d', positional=[], exact_positional=True) is rejected with "requires at least one of verb / positional / flags / tool to match on" even though positional IS set and exact_positional gives it meaning ("an invocation with zero non-flag arguments"). Adding tool='uip' makes the same config validate, which shows the rejection is an artifact of the falsiness test, not of the constraint being empty. This is the inverse half of the new exact_positional/positional guard at line 713. Change the positional term to self.positional is None and not self.exact_positional, or leave the behaviour and reword the message so it does not claim an explicitly-set field is unset.
3. [Axis 3] test_single_verb_detail_is_unchanged does not pin the 'renders exactly as it did before' claim — a multi-space verb now renders normalized (tests/test_cli_called_criterion.py:835) — The renderer changed from facets.append(f"verb={criterion.verb!r}") to facets.append(f"verb={' | '.join(' '.join(t) for t in criterion.verb_spellings)!r}") (src/coder_eval/criteria/cli_called.py:292), justified by the comment at 290-291: "A single verb renders exactly as it did before." That is not exactly true — the new path round-trips through split()/join(), so whitespace is normalized. Verified: for verb='ixp projects get' the old form renders 'ixp projects get' and the new form renders 'ixp projects get'. The guarding test at tests/test_cli_called_criterion.py:835-842 uses the single-spaced verb="ixp projects get" and asserts "verb='ixp projects get'" in (result.details or ""), so it cannot detect the divergence. Either parametrize that test with an irregularly-spaced verb and assert the normalized output (making the normalization deliberate), or soften the code comment to say the rendering is normalized rather than identical. Failure-detail text only — no score impact.

What's Missing

Parallel paths:

  • 🟡 docs/TASK_DEFINITION_GUIDE.md § cli_called (starts line 917) is untouched by this PR: its example still reads verb: "ixp projects configure-model" with the closing rationale at line 1024 calling verb an "ordered prefix", and exact_positional appears in no .md file in the repo — so the pydantic field descriptions are the only place a task author can learn either feature. (trigger: src/coder_eval/models/criteria.py) (restates: Axis 7: New exact_positional field and the list form of verb ship undocumented)
  • 🟡 The generated plugin criteria reference (plugins/coder-eval/reference/criteria.md, present on feat/claude-code-plugin, produced by make plugin-reference and guarded by CE033) carries a per-field table whose cli_called section still shows the pre-PR verb description and has no exact_positional row — whichever branch merges second must re-run make plugin-reference, or CE033 fails / the shipped plugin ships a stale schema reference. (trigger: src/coder_eval/models/criteria.py) (restates: Axis 7: New exact_positional field and the list form of verb ship undocumented)
  • 🔵 The cli_called row of the criteria table in CLAUDE.md still summarizes the criterion as "verb / positional / per-flag predicates, with min_count/max_count bounds" — neither verb alternation nor the exact-tail assertion is reflected in the repo's own capability index. (trigger: src/coder_eval/models/criteria.py) (restates: Axis 7: New exact_positional field and the list form of verb ship undocumented)
  • 🟡 The new verb_spellings property was adopted by the checker but its second call site was not migrated: _validate_bounds re-implements both the scalar/list normalization (criteria.py:679, identical to :662) and the .split() (criteria.py:698 vs :663), and the renderer still branches on criterion.verb is not None (cli_called.py:289) while the matcher branches on if spellings: (cli_called.py:154). (trigger: src/coder_eval/models/criteria.py) (restates: Axis 5: verb normalization + .split() duplicated in _validate_bounds)

Tests:

  • 🟠 No test in the repo passes verb spellings of differing length, so the new offset = len(matched) branch (cli_called.py:164) is never discriminated — mutating it to len(spellings[0]) still yields 95 passed on the whole new suite. (trigger: src/coder_eval/criteria/cli_called.py) (restates: Axis 3: offset-from-matched-spelling branch is never discriminated)
  • 🟡 The PR's two features are never tested together: every TestExactPositional case (tests lines 844-953) uses a string verb, and no test anywhere combines a list verb with exact_positional=True, even though both meet at the same offset arithmetic in _record_matches. (trigger: tests/test_cli_called_criterion.py) (restates: Axis 3: offset-from-matched-spelling branch is never discriminated)
  • 🟡 Nothing pins the order-independence claim the new code asserts ("at most one can match and this cannot depend on list order", cli_called.py:161-163) — the only coupling between the new prefix-collision validator and the offset derivation; a test asserting an identical score with the spellings list reversed would make that invariant executable. (trigger: src/coder_eval/criteria/cli_called.py) (restates: Axis 3: offset-from-matched-spelling branch is never discriminated)
  • 🟡 No test covers the false-FAIL shape exact_positional newly enables — an undeclared value-bearing flag (e.g. --folder Finance, absent from the default value_flags: ["output"]) leaves its value in positional and rejects a correct invocation; the added test_exact_positional_ignores_flags (tests line 895) uses --output json, which is in both value_flags and ignore_flags, i.e. the benign direction. (trigger: src/coder_eval/criteria/cli_called.py) (restates: Axis 8: exact_positional makes the verdict depend on value_flags completeness)
  • 🔵 The documented headline use of the new field — positional: [] + exact_positional: true to assert "the verb took no arguments" — has no test of the standalone form; today that config is rejected outright by the falsiness-based at-least-one-facet guard (criteria.py:721) unless another facet is also set. (trigger: src/coder_eval/models/criteria.py) (restates: Axis 2: not self.positional conflates positional: [] with unset)
  • 🔵 Both new schema arms are exercised only through direct Python construction — the test file never loads a TaskDefinition/YAML, and the discriminated-union payload map in tests/test_success_criterion_union.py still pins only the string verb, so nothing guards the surface task authors actually write (verb: [a, b], exact_positional: true in YAML). (trigger: tests/test_cli_called_criterion.py)

Downstream consumers:

  • 🔵 Scoring stays binary and cli_called keeps the default aggregate(), so no rate/threshold consumer needed updating — but the failure-detail string, which reports.py:892 renders verbatim as the per-criterion failure reason, silently changed for every existing single-verb config (whitespace now normalized through split()/join()), and the guarding test uses a single-spaced verb that cannot see it. (trigger: src/coder_eval/criteria/cli_called.py) (restates: Axis 3: test_single_verb_detail_is_unchanged does not pin the "renders exactly as before" claim)

Daily/nightly:

  • 🔵 Blast radius is unstated: no in-repo task or experiment YAML uses cli_called (grep -rl cli_called tasks/ experiments/ templates/ is empty), so the consumers are the out-of-tree UiPath ixp/uip CLI suites — the PR should say explicitly that both changes are additive for them (exact_positional defaults False; the string verb path is behavior-identical apart from normalized detail text) so nobody has to re-derive it before the next nightly. (trigger: src/coder_eval/models/criteria.py)

Harness & Lint Improvements

Static checks (lint / type):

  • [ruff] Enable ruff's mccabe gate: add "C90" to select and [tool.ruff.lint.mccabe] max-complexity = 15 in pyproject.toml, alongside the existing PLR0915/PLR0912 function-size ceilings (which this PR did NOT trip — _validate_bounds has 15 branches, under the max-branches=25 bar — which is exactly why complexity growth slipped through). Measured in both trees: CliCalledCriterion._validate_bounds is C901=10 on origin/main and C901=16 at PR HEAD fea3089, so a 15 ceiling fails this PR and passes its base. Pick the threshold on ruff's scale, not radon's (radon reports 29 for the same function because it counts boolean operators). Migration cost is bounded and already has repo precedent: uv run ruff check --select C901 --config lint.mccabe.max-complexity=15 src/ reports 14 pre-existing offenders (_build_argv 29, generate_variant_report 24, _simulation_dialog_loop 22, communicate 21, …); grandfather each with an explicit # noqa: C901 debt marker, the same 'gate NEW growth, mark existing debt' policy the pyproject comment already states for PLR0915/PLR0912. Prevents: Axis 1 medium _validate_bounds complexity C(19)->D(29) owning nine unrelated validation concerns (src/coder_eval/models/criteria.py:666), and its merged Axis 5 twin. Also would have flagged _record_matches / _check_impl growth in src/coder_eval/criteria/cli_called.py.
  • [ce-lint] New rule CE035 — 'no scalar-or-container unions on pydantic model fields'. AST rule over src/coder_eval/models/**.py: flag any AnnAssign on a BaseModel subclass whose annotation unions a scalar with a container of that scalar (str | list[str], list[str] | str, incl. under | None). Fix direction encoded in the message: declare the single shape (list[str] | None) and normalize the scalar shorthand ONCE in a @model_validator(mode="before"), the idiom already used at src/coder_eval/models/criteria.py:490-496 (FlagMatch._coerce_scalar_shorthand). Slots into tests/lint/rules/ce035_no_scalar_or_container_union.py + ALL_RULES in tests/lint/runner.py (CE034 is already reserved in .claude/harness-candidates.md:190 — do not reuse). Verified violation count: exactly ONE in the whole models package at PR HEAD (verb: str | list[str] | None, criteria.py:579) and ZERO on the base branch, so the rule lands green after this PR's fix. Note pyright cannot reach this: the union is well-typed; the defect is that one arm carries different SEMANTICS (alternation) than the sibling positional: list[str] (ordered token chain), which only a shape rule can forbid. Prevents: Axis 2 high — verb: str | list[str] list arm means alternation while sibling positional: list[str] means an ordered chain, so verb: ['ixp','projects','list'] validates and silently scores 1.0 on ixp projects delete (criteria.py:579). Also the merged Axis 1/2/5 medium — the same union forces the isinstance(self.verb, str) + .split() normalization to be duplicated at criteria.py:662 and :679, which is what makes verb_spellings' 'One place splits the field' docstring (L657-658) literally false.
  • [ce-lint] New rule CE036 — 'Optional container fields must be tested with is None, not falsiness'. AST rule over src/coder_eval/models/**.py: inside a @model_validator (or any method) of a BaseModel, flag not self.<f> / if self.<f>: where <f> is declared list[...] | None or dict[...] | None, because the test conflates 'unset' with 'explicitly empty'. Exempt (do not flag) a falsiness test already narrowed by an is not None term in the same BoolOp — that is the deliberate non-empty check at src/coder_eval/models/tasks.py:251 (self.paths is not None and not self.paths). I ran this scan over the PR tree: with that exemption it flags exactly ONE line, criteria.py:721, i.e. the finding and nothing else. Same file layout/wiring as CE035. Prevents: Merged Axis 2/7/8 low — not self.positional at src/coder_eval/models/criteria.py:721 rejects positional: [] + exact_positional=True with 'requires at least one of verb / positional / flags / tool to match on', even though positional IS set and exact_positional (whose own description at L608 advertises positional: []) gives it meaning.
  • [ce-lint] Extend CE030 (tests/lint/doc_schema_parity.py) to cover the criterion models. Today DOCUMENTED_MODELS registers only TaskDefinition / RunLimits / Dataset / SimulationConfig and the module docstring explicitly excludes 'criteria, …' from the walk — which is why make lint reported 177 passed on a PR that shipped a brand-new user-authored field with zero doc coverage. Add the 15 members of the SuccessCriterion union (enumerate them from typing.get_args of the annotated union, so a 16th criterion is auto-enrolled) paired with docs/TASK_DEFINITION_GUIDE.md, reusing the existing inline-code matcher and per-model EXEMPT map. I measured the migration cost against the PR tree: exactly TWO fields fail today — CliCalledCriterion.exact_positional (absent from all Markdown) and CliCalledCriterion.positional (appears only inside a fenced YAML block, never as inline code). Every other criterion field on every other criterion model already passes. Cost: two doc lines and the rule is green. Prevents: Axis 7 medium (merged with Axis 8) — exact_positional and the list form of verb ship undocumented; docs/TASK_DEFINITION_GUIDE.md §cli_called (L917-1024) still says verb is an 'ordered prefix' with no mention of alternation or the prefix-collision rejection. Also the documentation half of the Axis 8 medium (the value_flags prerequisite exact_positional silently depends on).
  • [ce-lint] New rule CE037 — 'every criterion-model field must be exercised by name in tests/'. Companion clause to the CE030 extension above, wired the same way (a dedicated @pytest.mark.lint test class, not a BaseRule, since it reasons over the model registry x the test sources rather than one AST): for each member of the SuccessCriterion union, assert every model_fields key appears as a word-boundary match somewhere under tests/, with an EXEMPT map carrying a reason. Measured cost on the PR tree: exactly ONE field in the entire criteria surface is never named in any test — CliCalledCriterion.value_flags — which is precisely the field whose completeness the new exact_positional verdict silently depends on. A field no test ever sets is a field whose interaction with new strictness flags cannot have been considered. Prevents: Axis 8 medium — exact_positional makes the verdict depend on value_flags completeness: an undeclared value-bearing flag (--folder Finance) turns a correct invocation into a false FAIL (src/coder_eval/criteria/cli_called.py:172), and no test sets value_flags at all (the added test_exact_positional_ignores_flags uses --output, which is in BOTH default lists — the benign direction).
  • [ce-lint] New rule CE038 — 'CI lint paths must equal the Makefile's LINT_PATHS'. Parse LINT_PATHS := src/ tests/ .github/scripts/ from the Makefile (line 22) and every ruff format --check ... / ruff check ... run: line in .github/workflows/pr-checks.yml, and fail on any set difference. Verified drift exists RIGHT NOW: pr-checks.yml lines 87/90 (Ubuntu) and 382/385 (Windows) lint src/ tests/ while make verify (Makefile:51) lints src/ tests/ .github/scripts/ — so a malformed file under .github/scripts/ passes CI green and reddens every contributor's local make verify, the exact inverse of the failure this PR hit. The alternative, cheaper fix is to have the workflow steps call make check / a new make format-check target so there is only one path list; the rule is the guard if the duplication is kept deliberately (Windows uses .venv/Scripts/). Prevents: Axis 1 medium — ruff format --check src/ tests/ fails on two PR-touched files (src/coder_eval/models/criteria.py:689 and tests/test_cli_called_criterion.py:952-954), which slipped because the local signal the author ran covered a narrower path set than the gate. Closes the 'the gate and the local check disagree about scope' class in both directions.

Harness improvements (not statically reachable):

  • Add a diff-scoped mutation-testing target, e.g. make mutate-diff running mutmut (or cosmic-ray) restricted to the files changed vs origin/main, with the test selection narrowed to the touched test modules; surface the surviving-mutant list in the review loop (not necessarily as a blocking CI job — cost). Minimum viable version: a documented one-liner in the review skill that mutates each new/changed non-trivial expression in the diff and re-runs the file's tests. The concrete signal it would have produced here: replacing offset = len(matched) with offset = len(spellings[0]) (src/coder_eval/criteria/cli_called.py:164) leaves uv run pytest tests/test_cli_called_criterion.py at 95 passed. Why not static: Requires executing the test suite against perturbed source. No AST rule can tell that two spellings passed to a test happen to be the same LENGTH, which is what makes the branch under test indistinguishable — the test text looks fully correct and its docstring even states the right contract. Prevents: Axis 3 high — the only test for the offset-from-matched-spelling branch (tests/test_cli_called_criterion.py:812-822) passes two 3-token spellings, so len(matched) == len(spellings[0]) and the branch is never discriminated; and Axis 3 low — test_single_verb_detail_is_unchanged (L835-842) uses a single-spaced verb so it cannot see the new render path's whitespace normalization.
  • Add a golden/snapshot artifact for criterion-facing TEXT: (a) every ValueError a criterion model's validators can raise, keyed by the rejected config, and (b) each criterion's details render for a representative pass and fail. Store as a committed golden file regenerated by a make target, so a message change shows up as a reviewable diff rather than hiding behind pytest.raises(match="is a prefix of"). Seed it with the irregular-whitespace verb ('ixp projects get') and the duplicate-entry verb (['a b','a b']) so both currently-wrong texts appear verbatim in the golden and must be signed off. Why not static: Message ACCURACY is semantic: no rule can know that 'is a prefix of' reads as a validator bug when the two operands are identical, that 'keep the shorter one alone' is nonsense for a duplicate, or that a rationale citing positional does not apply to a config that never set positional. A golden file cannot judge the text either — it makes the text a reviewed artifact instead of an invisible string, which is the reachable goal. Prevents: Merged Axis 1/6 low — duplicate-verb and positional-rationale messages at src/coder_eval/models/criteria.py:699-708 are wrong for the cases they fire on, and the guarding test asserts only a 4-word substring; Axis 3 low — the 'renders exactly as it did before' comment at criteria/cli_called.py:290-291 is false (split()/join() normalizes whitespace) and no test pins it.
  • Establish a 'hazard direction' convention for criterion strictness flags, enforced by a hypothesis property test on cli_called: for any argv that a criterion accepts, inserting an UNDECLARED value-bearing flag anywhere after the verb must not flip the verdict. That property fails today under exact_positional (verified: ['ixp','projects','list','proj-1','--folder','Finance'] scores 0.0 with positional=['proj-1'], exact_positional=True, and passes once folder is added to value_flags), which is the amplification the PR introduced — pre-change a stray token only mattered if it landed inside the graded slice. Whichever way the team resolves it (declare the coupling in the field description + guide, or make exact_positional count only declared-flag-stripped positionals), the property is the regression guard. Why not static: Needs the matcher executed over generated argv: whether a stray token lands inside positional[offset:offset+len(expected)] depends on runtime tokenization of a specific invocation, not on any statically visible shape. CE037 above can prove value_flags is exercised somewhere; only execution can prove it is exercised in the FAILING direction. Prevents: Axis 8 medium — an undeclared value-bearing flag turns a correct agent invocation into a false FAIL (0.0) under exact_positional (src/coder_eval/criteria/cli_called.py:172), an eval-harness scoring hazard: the agent ran exactly the asserted command.
  • Close the 'the formatter never ran on this machine' gap rather than adding another check: (a) add pre-push to default_install_hook_types in .pre-commit-config.yaml with a local hook running uv run ruff format --check $(LINT_PATHS) + uv run ruff check $(LINT_PATHS) with pass_filenames: false, so a branch cannot leave the machine unformatted even when individual commits were made with --no-verify or before make install ran the hook installer; and (b) have the review/implement skills run make format && make check (not a src/-only invocation) as the last step before handing a branch off. Why not static: The static check already exists and is correct (Makefile:51, pr-checks.yml:87/382) — the defect is purely about WHEN and over WHICH PATHS it executes on a contributor's machine. No lint rule can observe that git hooks were not installed or were bypassed; that is a workflow/harness property. Prevents: Axis 1 medium — ruff format --check fails on two PR-touched files (src/coder_eval/models/criteria.py:689, tests/test_cli_called_criterion.py:952-954), both clean on origin/main, reddening make verify and both CI format gates.

Top 5 Priority Actions

  1. Close the verb: list[str] alternation footgun at src/coder_eval/models/criteria.py:579 — verb: ['ixp','projects','list'] parses as three single-token alternatives and scores ixp projects delete --yes as a pass, so either move alternation to its own key (verb_any_of) leaving verb a plain str, or reject/flag suspicious single-token alternation lists, and add the confusable case to TestVerbAlternationValidation.
  2. Document and test the exact_positional × value_flags coupling at src/coder_eval/criteria/cli_called.py:172 — with exact_positional: true, any undeclared value-bearing flag (e.g. --folder Finance) leaves its value in positional and turns the exactly-correct invocation into a 0.0, so state the prerequisite in the field description (src/coder_eval/models/criteria.py:605-612) and add the false-FAIL test the current test_exact_positional_ignores_flags (tests/test_cli_called_criterion.py:895) misses.
  3. Make the offset-from-matched-spelling test actually discriminate at tests/test_cli_called_criterion.py:819 — both spellings are 3 tokens, so mutating offset = len(matched) (src/coder_eval/criteria/cli_called.py:164) to len(spellings[0]) still passes all 95 tests; use genuinely differing-length spellings (['ixp projects get', 'ixp get']), mirror the case that matches the other spelling, and add the untested list-verb × exact_positional combination.
  4. Run make format and commit — ruff format --check src/ tests/ currently reformats two PR-touched files (src/coder_eval/models/criteria.py:689 and tests/test_cli_called_criterion.py:952-954), turning make verify and both the Ubuntu (.github/workflows/pr-checks.yml:87) and Windows (line 382) CI format gates red for a zero-behavior change.
  5. Pay down the cli_called schema debt in one pass: document verb's list form, its prefix-collision rule, and exact_positional in docs/TASK_DEFINITION_GUIDE.md § cli_called (line 917, untouched by this PR and the only user-facing reference), then normalize verb once via a mode="before" validator like FlagMatch._coerce_scalar_shorthand (src/coder_eval/models/criteria.py:490) so _validate_bounds (line 666, now CC 29 across nine rules) stops re-deriving what verb_spellings claims to own, splitting the verb rules into their own validator and fixing the duplicate-entry / positional: [] message inaccuracies at lines 702 and 721.

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants