feat(cli-called): accept a list of verb spellings - #103
Conversation
`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>
|
Claude finished @alexandrujircan's task in 1m 27s —— View job Code Review In Progress
|
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
left a comment
There was a problem hiding this comment.
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
- [Axis 2]
verb: str | list[str]list arm means alternation while the siblingpositional: 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 toverb: 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
- [Axis 1] 🟡
ruff format --checkfails on two files this PR touches —make verifyand the CI format gate (pr-checks.yml, Ubuntu + Windows) go red (src/coder_eval/models/criteria.py:689) — Verified withuv run ruff format --check src/ tests/(the exactLINT_PATHSat Makefile:22, used byverify: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
- [Axis 1] New verb-list validator messages misdescribe the duplicate-entry case and cite a
positionaloffset/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 readsverb: "ixp projects configure-model"with the closing rationale at line 1024 callingverban "ordered prefix", andexact_positionalappears in no.mdfile 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: Newexact_positionalfield and the list form ofverbship undocumented) - 🟡 The generated plugin criteria reference (
plugins/coder-eval/reference/criteria.md, present onfeat/claude-code-plugin, produced bymake plugin-referenceand guarded by CE033) carries a per-field table whosecli_calledsection still shows the pre-PRverbdescription and has noexact_positionalrow — whichever branch merges second must re-runmake plugin-reference, or CE033 fails / the shipped plugin ships a stale schema reference. (trigger: src/coder_eval/models/criteria.py) (restates: Axis 7: Newexact_positionalfield and the list form ofverbship undocumented) - 🔵 The
cli_calledrow of the criteria table inCLAUDE.mdstill 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: Newexact_positionalfield and the list form ofverbship undocumented) - 🟡 The new
verb_spellingsproperty was adopted by the checker but its second call site was not migrated:_validate_boundsre-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 oncriterion.verb is not None(cli_called.py:289) while the matcher branches onif spellings:(cli_called.py:154). (trigger: src/coder_eval/models/criteria.py) (restates: Axis 5:verbnormalization +.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 tolen(spellings[0])still yields95 passedon 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
TestExactPositionalcase (tests lines 844-953) uses a stringverb, and no test anywhere combines a listverbwithexact_positional=True, even though both meet at the sameoffsetarithmetic 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_positionalnewly enables — an undeclared value-bearing flag (e.g.--folder Finance, absent from the defaultvalue_flags: ["output"]) leaves its value inpositionaland rejects a correct invocation; the addedtest_exact_positional_ignores_flags(tests line 895) uses--output json, which is in bothvalue_flagsandignore_flags, i.e. the benign direction. (trigger: src/coder_eval/criteria/cli_called.py) (restates: Axis 8:exact_positionalmakes the verdict depend onvalue_flagscompleteness) - 🔵 The documented headline use of the new field —
positional: []+exact_positional: trueto 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.positionalconflatespositional: []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 intests/test_success_criterion_union.pystill pins only the stringverb, so nothing guards the surface task authors actually write (verb: [a, b],exact_positional: truein YAML). (trigger: tests/test_cli_called_criterion.py)
Downstream consumers:
- 🔵 Scoring stays binary and
cli_calledkeeps the defaultaggregate(), so no rate/threshold consumer needed updating — but the failure-detail string, whichreports.py:892renders verbatim as the per-criterion failure reason, silently changed for every existing single-verb config (whitespace now normalized throughsplit()/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_unchangeddoes 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 UiPathixp/uipCLI suites — the PR should say explicitly that both changes are additive for them (exact_positionaldefaults False; the stringverbpath 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
selectand[tool.ruff.lint.mccabe] max-complexity = 15in pyproject.toml, alongside the existing PLR0915/PLR0912 function-size ceilings (which this PR did NOT trip —_validate_boundshas 15 branches, under the max-branches=25 bar — which is exactly why complexity growth slipped through). Measured in both trees:CliCalledCriterion._validate_boundsis 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_argv29,generate_variant_report24,_simulation_dialog_loop22,communicate21, …); grandfather each with an explicit# noqa: C901debt marker, the same 'gate NEW growth, mark existing debt' policy the pyproject comment already states for PLR0915/PLR0912. Prevents: Axis 1 medium_validate_boundscomplexity 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_implgrowth 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 anyAnnAssignon aBaseModelsubclass 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 siblingpositional: 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 siblingpositional: list[str]means an ordered chain, soverb: ['ixp','projects','list']validates and silently scores 1.0 onixp projects delete(criteria.py:579). Also the merged Axis 1/2/5 medium — the same union forces theisinstance(self.verb, str)+.split()normalization to be duplicated at criteria.py:662 and :679, which is what makesverb_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 oversrc/coder_eval/models/**.py: inside a@model_validator(or any method) of a BaseModel, flagnot self.<f>/if self.<f>:where<f>is declaredlist[...] | Noneordict[...] | None, because the test conflates 'unset' with 'explicitly empty'. Exempt (do not flag) a falsiness test already narrowed by anis not Noneterm in the sameBoolOp— 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.positionalat src/coder_eval/models/criteria.py:721 rejectspositional: [] + exact_positional=Truewith 'requires at least one of verb / positional / flags / tool to match on', even thoughpositionalIS set andexact_positional(whose own description at L608 advertisespositional: []) gives it meaning. - [ce-lint] Extend CE030 (tests/lint/doc_schema_parity.py) to cover the criterion models. Today
DOCUMENTED_MODELSregisters only TaskDefinition / RunLimits / Dataset / SimulationConfig and the module docstring explicitly excludes 'criteria, …' from the walk — which is whymake lintreported 177 passed on a PR that shipped a brand-new user-authored field with zero doc coverage. Add the 15 members of theSuccessCriterionunion (enumerate them fromtyping.get_argsof 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) andCliCalledCriterion.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_positionaland the list form ofverbship undocumented; docs/TASK_DEFINITION_GUIDE.md §cli_called(L917-1024) still saysverbis an 'ordered prefix' with no mention of alternation or the prefix-collision rejection. Also the documentation half of the Axis 8 medium (thevalue_flagsprerequisiteexact_positionalsilently 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.linttest class, not a BaseRule, since it reasons over the model registry x the test sources rather than one AST): for each member of theSuccessCriterionunion, assert everymodel_fieldskey appears as a word-boundary match somewhere undertests/, 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 newexact_positionalverdict 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_positionalmakes the verdict depend onvalue_flagscompleteness: 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 setsvalue_flagsat all (the addedtest_exact_positional_ignores_flagsuses--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 everyruff 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) lintsrc/ tests/whilemake verify(Makefile:51) lintssrc/ tests/ .github/scripts/— so a malformed file under.github/scripts/passes CI green and reddens every contributor's localmake verify, the exact inverse of the failure this PR hit. The alternative, cheaper fix is to have the workflow steps callmake check/ a newmake format-checktarget 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-diffrunning mutmut (or cosmic-ray) restricted to the files changed vsorigin/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: replacingoffset = len(matched)withoffset = len(spellings[0])(src/coder_eval/criteria/cli_called.py:164) leavesuv run pytest tests/test_cli_called_criterion.pyat 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, solen(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
ValueErrora criterion model's validators can raise, keyed by the rejected config, and (b) each criterion'sdetailsrender for a representative pass and fail. Store as a committed golden file regenerated by amaketarget, so a message change shows up as a reviewable diff rather than hiding behindpytest.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 citingpositionaldoes not apply to a config that never setpositional. 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 andpositional-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 underexact_positional(verified:['ixp','projects','list','proj-1','--folder','Finance']scores 0.0 withpositional=['proj-1'], exact_positional=True, and passes oncefolderis added tovalue_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 makeexact_positionalcount 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 insidepositional[offset:offset+len(expected)]depends on runtime tokenization of a specific invocation, not on any statically visible shape. CE037 above can provevalue_flagsis 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) underexact_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-pushtodefault_install_hook_typesin .pre-commit-config.yaml with a local hook runninguv run ruff format --check $(LINT_PATHS)+uv run ruff check $(LINT_PATHS)withpass_filenames: false, so a branch cannot leave the machine unformatted even when individual commits were made with--no-verifyor beforemake installran the hook installer; and (b) have the review/implement skills runmake format && make check(not asrc/-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 --checkfails 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, reddeningmake verifyand both CI format gates.
Top 5 Priority Actions
- 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 scoresixp projects delete --yesas a pass, so either move alternation to its own key (verb_any_of) leavingverba plainstr, or reject/flag suspicious single-token alternation lists, and add the confusable case toTestVerbAlternationValidation. - Document and test the
exact_positional×value_flagscoupling at src/coder_eval/criteria/cli_called.py:172 — withexact_positional: true, any undeclared value-bearing flag (e.g.--folder Finance) leaves its value inpositionaland 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 currenttest_exact_positional_ignores_flags(tests/test_cli_called_criterion.py:895) misses. - 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) tolen(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_positionalcombination. - Run
make formatand 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), turningmake verifyand both the Ubuntu (.github/workflows/pr-checks.yml:87) and Windows (line 382) CI format gates red for a zero-behavior change. - Pay down the
cli_calledschema debt in one pass: documentverb's list form, its prefix-collision rule, andexact_positionalin docs/TASK_DEFINITION_GUIDE.md §cli_called(line 917, untouched by this PR and the only user-facing reference), then normalizeverbonce via amode="before"validator likeFlagMatch._coerce_scalar_shorthand(src/coder_eval/models/criteria.py:490) so_validate_bounds(line 666, now CC 29 across nine rules) stops re-deriving whatverb_spellingsclaims 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.

cli_called.verbnow accepts a list of spellings, matching if any entry does.The problem
verbwas 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 amax_count: 0guard (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":projects getprojects listprojects deleteprojects update-titleprojects publishprojects fetch-meta(hallucinated)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
The docstring now states the breadth asymmetry it left implicit — order was documented ("
labellings confirmnever matcheslabellings unconfirm"), widening was not.Note "exact verb matching" is deliberately not what this adds:
cli_calledhas no CLI grammar, so["ixp","projects","get","proj-1"]is the same argv whethergetis a verb token or a positional. The author supplies the boundary — a fuller verb, orpositional. 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:
" ".split()is an empty prefix matching everythingpositionaloffset would depend on list order. Catches duplicates too, being prefixes of themselves.Tests
27 new, including:
max_count: 0guard 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.projects listmust not matchprojects listsorprojects list-models. Previously untested, and it's the property that makes "list full verbs" a sufficient answer rather than a partial one.positionaloffset following the matched spelling when lengths differ.a | brather than a bare repr, which would read as "the verb is these tokens".Gate
tests/test_cli_called_criterion.py: 87 passedmain(3 ×reports_stats_nonfinitefloat/numerator, 2 ×test_sandboxWindows symlink privilege, 3 ×claude_settings_enforcement_live). Verified by running them in a worktree at57556af.make lint: 177 passedBackwards compatible:
verbas a string behaves exactly as before, including its failure-detail text.🤖 Generated with Claude Code