feat(cli-called): accept alternative verbs via verb_any_of - #103
feat(cli-called): accept alternative verbs via verb_any_of#103alexandrujircan wants to merge 10 commits into
Conversation
|
Claude finished @alexandrujircan's task in 1m 27s —— View job Code Review In Progress
|
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.
Review on #103 found the list arm of `verb: str | list[str]` reintroduced the very fail-open this PR set out to close. `verb: ["ixp", "projects", "list"]` — the natural way to mistype a chain — parsed as three single-token ALTERNATIVES, and the bare `ixp` entry is a one-token prefix matching every uip call, so it scored 1.0 on `ixp projects delete`. Reproduced before fixing. No validator can separate that from a legitimate `["list", "ls"]`, so the shape is gone from the schema: `verb` is a plain `str` again and alternation lives in `verb_any_of`, making the mistyped form a pydantic type error. Also from the review, each reproduced first: - The offset-from-matched-spelling test used two 3-token spellings, so the branch was never discriminated — mutating `offset = len(matched)` to `len(spellings[0])` left all 95 tests green. Now parametrized over genuinely differing lengths (`["ixp projects get", "ixp get"]`), both spellings exercised, plus the inverse and an order-independence case. The mutation now kills two tests. - `exact_positional` silently depends on `value_flags` completeness: an undeclared flag is read as a switch, so `--folder Finance` leaves its VALUE among the positionals and turns an exactly-correct invocation into 0.0. Stated on the field and pinned by a test asserting both directions. The previous test used `--output`, which is in both default lists — the benign direction. - `not self.positional` conflated `positional: []` with unset, rejecting `positional: [] + exact_positional` — the field's own documented headline use — as "requires at least one of ...". - The duplicate-entry message read "'a b' is a prefix of 'a b' ... keep the shorter one alone", which reads as a validator bug. Duplicates get their own message, and the prefix message no longer cites a `positional` the config may not have set. - `ruff format --check` was red on two files: `make format` had only ever been run over a narrower path set than `make verify` lints. Verb rules moved to their own `_validate_verb` so `_validate_bounds` stops growing, and both it and the failure detail now read `verb_spellings`, making that property's "one place splits the field" docstring true. Detail rendering normalizes whitespace, now stated in the comment and covered by a parametrized test rather than claimed to be identical. Docs: TASK_DEFINITION_GUIDE.md § cli_called gains `verb_any_of`, `exact_positional`, both hazard directions and the `value_flags` prerequisite; the CLAUDE.md criteria row names both new fields. 104 in the criterion file, 4036 in the full suite (same 8 pre-existing failures), make lint 177, ruff format/check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both blockers and the format gate are fixed in Blocker 1 — the list arm reintroduced the fail-open this PR exists to closeYou're right, and it's the worst version of the bug. Reproduced:
Agreed it isn't statically separable from a legitimate
Blocker 2 — the offset test didn't discriminateConfirmed, including your mutation: both spellings were 3 tokens, so Now parametrized over The missing cross-feature case is in too — list Format gate
The
|
|
Removed Across the consuming IXP suite: 6 criteria are Kept two pieces of that work, since neither depends on the field:
Trailing arguments are unconstrained again, now a stated property of
Gate: 95 in the criterion file, 4027 full suite (same 8 pre-existing), lint 177, format/check clean. |
`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>
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>
Review on #103 found the list arm of `verb: str | list[str]` reintroduced the very fail-open this PR set out to close. `verb: ["ixp", "projects", "list"]` — the natural way to mistype a chain — parsed as three single-token ALTERNATIVES, and the bare `ixp` entry is a one-token prefix matching every uip call, so it scored 1.0 on `ixp projects delete`. Reproduced before fixing. No validator can separate that from a legitimate `["list", "ls"]`, so the shape is gone from the schema: `verb` is a plain `str` again and alternation lives in `verb_any_of`, making the mistyped form a pydantic type error. Also from the review, each reproduced first: - The offset-from-matched-spelling test used two 3-token spellings, so the branch was never discriminated — mutating `offset = len(matched)` to `len(spellings[0])` left all 95 tests green. Now parametrized over genuinely differing lengths (`["ixp projects get", "ixp get"]`), both spellings exercised, plus the inverse and an order-independence case. The mutation now kills two tests. - `exact_positional` silently depends on `value_flags` completeness: an undeclared flag is read as a switch, so `--folder Finance` leaves its VALUE among the positionals and turns an exactly-correct invocation into 0.0. Stated on the field and pinned by a test asserting both directions. The previous test used `--output`, which is in both default lists — the benign direction. - `not self.positional` conflated `positional: []` with unset, rejecting `positional: [] + exact_positional` — the field's own documented headline use — as "requires at least one of ...". - The duplicate-entry message read "'a b' is a prefix of 'a b' ... keep the shorter one alone", which reads as a validator bug. Duplicates get their own message, and the prefix message no longer cites a `positional` the config may not have set. - `ruff format --check` was red on two files: `make format` had only ever been run over a narrower path set than `make verify` lints. Verb rules moved to their own `_validate_verb` so `_validate_bounds` stops growing, and both it and the failure detail now read `verb_spellings`, making that property's "one place splits the field" docstring true. Detail rendering normalizes whitespace, now stated in the comment and covered by a parametrized test rather than claimed to be identical. Docs: TASK_DEFINITION_GUIDE.md § cli_called gains `verb_any_of`, `exact_positional`, both hazard directions and the `value_flags` prerequisite; the CLAUDE.md criteria row names both new fields. 104 in the criterion file, 4036 in the full suite (same 8 pre-existing failures), make lint 177, ruff format/check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing needs it. Across the consuming IXP suite: 6 criteria are max_count 0 guards, where tightening lets the forbidden call evade; 2 set no positional, so it does not apply; the remaining 4 gain nothing, since an agent appending a stray positional to `configure-model proj-1 --model X` is not a realistic failure and the real CLI rejects it anyway. Zero of 12 would set the field. For that it charged two hazards — the negative-guard inversion, and a dependence on `value_flags` completeness that turns a correct invocation into 0.0 when an undeclared flag leaves its value among the positionals. Documenting a hazard is not the same as it being worth carrying. Review also found a bug in the field's own interaction with `positional: []`, which is the complexity cost showing up early. It was built in answer to "does `projects list dummy` still score?" — a question about the matcher's semantics, answered with a schema field before checking whether any assertion needed one. An opt-in boolean is purely additive, so adding it later breaks nothing; shipping it now makes every future reader reason about it. `verb_any_of` stays: it fixes a live hole, replacing the interim `projects delete` guard in the IXP suite that exists precisely because alternation was inexpressible. Kept from the removed work, since the trap is real without the field: `positional: []` is now REJECTED rather than silently asserting nothing (matching slices an empty expectation and compares it to itself). The at-least-one-facet check returns to plain falsiness, which is what catches `verb: ""`. Also kept: the whitespace-normalization test, which pins a rendering change to every existing single-verb config and is unrelated to the reverted field. 95 in the criterion file, 4027 in the full suite (same 8 pre-existing failures), make lint 177, ruff format/check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment volume, not content. Cut the restatements of what the line does, the examples now carried by tests, and the sentences that paraphrased the error message two lines below. Kept the non-obvious reasons: why the offset comes from the matched spelling, why falsiness rather than `is None` in the facet check, why a character count would pass a blank verb, and why alternation is its own key. Dropped the prefix-collision comment entirely — its error message already says the same thing, better. Net -37 lines across the two files, no behavior change. 95 in the criterion file, 4027 in the full suite, make lint 177. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ines It read "Spellings may differ in length; validation rules out two matching the same argv, so this cannot depend on list order" — two unrelated facts joined by a semicolon, with "this" pointing at nothing in particular. They explain different lines. That no argv can match two spellings is why `next()` taking the FIRST match is deterministic, so it belongs with the match. That lengths differ is why the offset comes from `matched` rather than a fixed entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Same source as the matcher, so the detail cannot describe a different constraint than the one applied" stated a property without naming what it prevents, and left "same source" as something the reader had to work out. The concrete failure: reading `criterion.verb` here is None for a `verb_any_of` criterion, so the detail would list no verb at all — omitting the constraint that caused the failure from the message whose job is to explain it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c34f324 to
d1b5974
Compare
…ites
Removing exact_positional left its two prerequisite bullets behind. One was dead,
the other was true for the wrong reason.
Dropped "not on a negative guard" — nothing tightens any more, so it described a
setting that no longer exists.
Kept and corrected the value_flags one. The coupling is NOT specific to exactness and
survives the removal: an undeclared flag is a switch, so its value stays non-flag and
takes the slot the criterion named. Verified — `get proj-1 --folder Finance` matches
`positional: ["proj-1"]`, but `get --folder Finance proj-1` scores 0.0. The trigger is
flag ORDER, not exactness, so the old wording ("turns an otherwise exactly-correct
invocation into 0.0") pointed at the wrong cause.
Now pinned by a test, and the guide states why the ambiguity resolves this way:
guessing that an unknown flag consumes the next token let `--yes proj-1` bind
`yes=proj-1` and swallow the project name, passing a delete guard on the delete it
forbade.
96 in the criterion file, 4028 in the full suite, make lint 177.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`verb`: "For one operation the tool spells several ways, use verb_any_of" is a garden-path sentence. `positional`: it called itself a prefix and then illustrated the point with 'projects list' vs 'projects list dummy' — an example containing no positional at all, so it demonstrated verb-prefix looseness on a field about arguments. Uses a real positional now, and names the value_flags dependency that shifts these slots. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review suggestion from @cezara98t. "all 14 of its subcommands" is a point-in-time fact about one CLI version — the catalog is refreshed by a bot, so the number rots while the sentence still reads as authoritative. The argument does not need it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Adds
verb_any_of— alternative whole verbs, matching if any entry does — for one operation the tool spells several ways. Opt-in; existing criteria are unaffected.Why
verbis an ordered prefix with no alternation, so a criterion needing "list OR get" had one option: truncate to the common prefix. That leaves the following tokens unconstrained — safe for amax_count: 0guard (it fires on more), wrong for a positive assertion.Found in a real task, where a weight-3.0 criterion asserting the agent read a project was written
verb: "ixp projects":verb_any_ofprojects get/projects listprojects deleteprojects update-titleprojects fetch-meta(hallucinated)The regex it replaced said
(list|get)and admitted none of those, so migrating that criterion to structured matching was a regression — and the API was why: the unsafe option was the only expressible one.UiPath/skills#2565carries an interimprojects deleteguard that exists solely because alternation was inexpressible; it comes out once this ships.The docstring also gains the breadth asymmetry it left implicit. It documented that order matters (
labellings confirm≠labellings unconfirm) but never that a shorter verb admits everything deeper, nor that the risk direction flips:max_count: 0guardWhy alternation is its own key, not a list arm on
verbThe first revision made
verbacceptstr | list[str]. Review caught that it reintroduced the fail-open this PR exists to close:Not statically separable from a legitimate
["list", "ls"], so the shape is gone from the schema rather than guarded:verbstays a plainstr, and the mistyped form is a pydantic type error.What this deliberately does not add
Exact verb matching.
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, via a fuller verb or viapositional.An exact-tail flag. An earlier revision added
exact_positional, and it was removed: across the consuming suite, 6 criteria aremax_count: 0guards where tightening lets the forbidden call evade, 2 set nopositionalso it cannot apply, and the remaining 4 gain nothing real. Zero of 12 would have set it, while it charged two hazards — the guard inversion, and a dependence onvalue_flagscompleteness that turns a correct invocation into0.0when an undeclared flag leaves its value among the positionals. It is purely additive, so it can return when something actually needs it.Trailing arguments therefore stay unconstrained, now a stated property of
positionalwith a test pinning it. Its removal also surfaced that thevalue_flagscoupling the review flagged is NOT specific to exactness — an undeclared flag before a positional shifts the slot the criterion named, soget --folder Finance proj-1missespositional: ["proj-1"]. Corrected in the guide and pinned by a test.Validation
Each of these either matches every record or resolves by list order:
verbandverb_any_oftogether, and an emptyverb_any_of(falsy, so it slipped past the at-least-one-facet check and read as "no verb constraint")" ".split()is an empty prefixpositional: [], kept from the removed work because the trap is real without the field: it slices an empty expectation and compares it to itself, so it reads as "took no arguments" and asserts nothingTests
max_count: 0guard must fire on every listed spelling, not just the first.projects listmust not matchprojects listsorprojects list-models. Previously untested, and it's what makes "list full verbs" a sufficient answer rather than a partial one.offset = len(matched)→len(spellings[0])survived all 95 tests. Now parametrized over differing lengths (["ixp projects get", "ixp get"]) with both exercised; the mutation kills two tests.Gate
tests/test_cli_called_criterion.py: 96 passedmainworktree at57556af(3 ×reports_stats_nonfinitefloat/numerator, 2 ×test_sandboxWindows symlink privilege, 3 ×claude_settings_enforcement_live)make lint177 passed ·ruff format --check+ruff checkover the fullLINT_PATHS: cleanopenai_codeximport errors onlyDocs:
TASK_DEFINITION_GUIDE.md§cli_calledcoversverb_any_of, why not to shorten a verb instead, and that the tail stays open; theCLAUDE.mdcriteria row names the field.Blast radius: no in-repo task uses
cli_called. The consumer is the migrated IXP suite inUiPath/skills#2565, which usesverbas a string only — nothing there breaks.Not addressed:
make plugin-reference/ CE033 (the generated reference lives onfeat/claude-code-plugin; whichever branch lands second owns the regen), and the review's five harness proposals (CE035–CE038, mccabeC90, a diff-scoped mutation target, a golden-text artifact, a pre-push format hook) — each is a repo-wide gate with its own migration cost, so they belong in their own PR. CE035 would have caught thestr | list[str]hazard statically.🤖 Generated with Claude Code