Skip to content

feat(cli-called): accept alternative verbs via verb_any_of - #103

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

feat(cli-called): accept alternative verbs via verb_any_of#103
alexandrujircan wants to merge 10 commits into
mainfrom
feat/cli-called-verb-alternation

Conversation

@alexandrujircan

@alexandrujircan alexandrujircan commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

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.

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

Why

verb is 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 a max_count: 0 guard (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":

agent ran before with verb_any_of
projects get / projects list 1.0 1.0
projects delete 1.0 0.0
projects update-title 1.0 0.0
projects fetch-meta (hallucinated) 1.0 0.0

The regex it replaced said (list|get) and admitted none of those, so migrating that criterion to structured matching was a regression — and the API was why: the unsafe option was the only expressible one. UiPath/skills#2565 carries an interim projects delete guard 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 confirmlabellings unconfirm) but never that a shorter verb admits everything deeper, nor that the risk direction flips:

short verb full verb
positive assertion unsafe — credits wrong calls safe
max_count: 0 guard safe — fires on more safe

Why alternation is its own key, not a list arm on verb

The first revision made verb accept str | list[str]. Review caught that it reintroduced the fail-open this PR exists to close:

verb: ["ixp", "projects", "list"]   # the natural way to mistype a chain
  → parses as three single-token ALTERNATIVES
  → the bare `ixp` entry is a one-token prefix matching every uip call
  → scores 1.0 on  ixp projects delete proj-1 --yes

Not statically separable from a legitimate ["list", "ls"], so the shape is gone from the schema rather than guarded: verb stays a plain str, and the mistyped form is a pydantic type error.

What this deliberately does not add

Exact verb matching. cli_called has no CLI grammar, so ["ixp","projects","get","proj-1"] is the same argv whether get is a verb token or a positional — the author supplies the boundary, via a fuller verb or via positional.

An exact-tail flag. An earlier revision added exact_positional, and it was removed: across the consuming suite, 6 criteria are max_count: 0 guards where tightening lets the forbidden call evade, 2 set no positional so 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 on value_flags completeness that turns a correct invocation into 0.0 when 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 positional with a test pinning it. Its removal also surfaced that the value_flags coupling the review flagged is NOT specific to exactness — an undeclared flag before a positional shifts the slot the criterion named, so get --folder Finance proj-1 misses positional: ["proj-1"]. Corrected in the guide and pinned by a test.

Validation

Each of these either matches every record or resolves by list order:

  • verb and verb_any_of together, and an empty verb_any_of (falsy, so it slipped past the at-least-one-facet check and read as "no verb constraint")
  • blank entries per item — " ".split() is an empty prefix
  • one entry prefixing another — the shorter already accepts everything the longer does; duplicates get their own message rather than "'a b' is a prefix of 'a b'"
  • positional: [], 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 nothing

Tests

  • The inverse of every matching change: a max_count: 0 guard must fire on every listed spelling, not just the first.
  • Token-by-token comparisonprojects list must not match projects lists or projects list-models. Previously untested, and it's what makes "list full verbs" a sufficient answer rather than a partial one.
  • A discriminating offset test. The first revision's used two 3-token spellings, so mutating 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.
  • Order-independence, and the whitespace normalization the new detail rendering introduces for existing single-verb configs.

Gate

  • tests/test_cli_called_criterion.py: 96 passed
  • Full suite: 4028 passed, 8 failed — all 8 verified identical on a clean main worktree at 57556af (3 × reports_stats_nonfinite float/numerator, 2 × test_sandbox Windows symlink privilege, 3 × claude_settings_enforcement_live)
  • make lint 177 passed · ruff format --check + ruff check over the full LINT_PATHS: clean
  • pyright: the 3 pre-existing openai_codex import errors only

Docs: TASK_DEFINITION_GUIDE.md § cli_called covers verb_any_of, why not to shorten a verb instead, and that the tail stays open; the CLAUDE.md criteria row names the field.

Blast radius: no in-repo task uses cli_called. The consumer is the migrated IXP suite in UiPath/skills#2565, which uses verb as a string only — nothing there breaks.

Not addressed: make plugin-reference / CE033 (the generated reference lives on feat/claude-code-plugin; whichever branch lands second owns the regen), and the review's five harness proposals (CE035–CE038, mccabe C90, 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 the str | list[str] hazard statically.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Aug 11, 2026

Copy link
Copy Markdown

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


Code Review In Progress

Todo List:

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

Starting review...

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

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

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

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

Summary

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

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

Blockers

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

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

Non-blocking, but please consider before merge

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

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

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

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

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

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

ruff wants the call collapsed onto one line.

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

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

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

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

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

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

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

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

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

Nits

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

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

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

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

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

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

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

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

What's Missing

Parallel paths:

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

Tests:

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

Downstream consumers:

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

Daily/nightly:

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

Harness & Lint Improvements

Static checks (lint / type):

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

Harness improvements (not statically reachable):

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

Top 5 Priority Actions

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

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

alexandrujircan added a commit that referenced this pull request Aug 12, 2026
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>
@alexandrujircan

Copy link
Copy Markdown
Contributor Author

Both blockers and the format gate are fixed in 65c95aa. I reproduced every finding before touching anything; all of them held.

Blocker 1 — the list arm reintroduced the fail-open this PR exists to close

You're right, and it's the worst version of the bug. Reproduced:

>>> c = CliCalledCriterion(description='d', log=LOG, verb=['ixp','projects','list'])
>>> c.verb_spellings
[['ixp'], ['projects'], ['list']]
>>> score on  ixp projects delete proj-1 --yes  ->  1.0

verb: ["ixp", "projects", "list"] is the natural way to mistype a chain, and as an alternation the bare ixp entry is a one-token prefix matching every uip call. So the API I added to stop over-matching had a shape that over-matched harder than the thing it replaced.

Agreed it isn't statically separable from a legitimate ["list", "ls"], so I took your first option: verb is a plain str again and alternation lives in verb_any_of. The mistyped form is now a pydantic type error rather than a silent 1.0:

Input should be a valid string [type=string_type, input_value=['ixp', 'projects', 'list']]

TestVerbAlternationValidation carries the confusable case with the reasoning in its docstring, plus a verb + verb_any_of mutual-exclusion test.

Blocker 2 — the offset test didn't discriminate

Confirmed, including your mutation: both spellings were 3 tokens, so len(matched) and len(spellings[0]) were identical and 95 passed survived the mutant.

Now parametrized over ["ixp projects get", "ixp get"] with both spellings exercised, so neither is the one that happens to be first, plus the inverse (wrong positional must still fail under the shorter spelling) and the order-independence case you noted was missing. Re-running your mutation now kills two tests:

FAILED test_positional_offset_follows_the_matched_spelling[two-token-spelling] - assert 0.0 == 1.0
FAILED test_score_is_independent_of_spelling_order                            - assert 0.0 == 1.0

The missing cross-feature case is in too — list verb_any_of combined with exact_positional.

Format gate

make format reformatted the two files. Worth recording the root cause you identified: I had been running a narrower path set than make verify lints, which is also why the Windows Smoke Test was red — same single cause, 2 files would be reformatted. Both gates are green locally now.

The exact_positional × value_flags coupling

This was the finding I was most glad to get, because it's a scoring hazard rather than a style issue, and my own test used --output — in both default lists, i.e. the benign direction. Reproduced:

list proj-1 --folder Finance                        -> 0.0
same, with folder declared in value_flags           -> 1.0

The agent ran exactly the asserted command and scored 0.0. Now stated in the field description as a prerequisite, and pinned by a test asserting both directions.

Also fixed

  • not self.positional conflated [] with unset — so positional: [] + exact_positional, the field's own documented headline use, was rejected as "requires at least one of …". The positional term now tests is None when exact_positional is set; falsiness stays for verb/flags/tool, where it deliberately catches verb: "".
  • Message accuracy — duplicates get their own message (verb_any_of lists 'a b' twice) instead of "'a b' is a prefix of 'a b' … keep the shorter one alone", and the prefix message no longer cites a positional the config may never have set. Its rationale is now the true one: the shorter entry already accepts everything the longer does.
  • The duplicated normalization — verb rules moved to their own _validate_verb, and both it and the failure detail read verb_spellings, so that property's "one place splits the field" docstring is finally true. The renderer also branches on the same source as the matcher rather than on verb is not None.
  • Whitespace normalization — you're right that "renders exactly as it did before" was false. The comment now says normalized, and the test is parametrized with 'ixp projects get' so the behavior is deliberate rather than incidental.
  • DocsTASK_DEFINITION_GUIDE.md § cli_called gains verb_any_of, exact_positional, both hazard directions and the value_flags prerequisite; the closing rationale now says the prefix is compared token by token and names what closes the tail. CLAUDE.md's criteria row names both new fields.

Deferred, with reasons

  • make plugin-reference / CE033 — the generated reference lives on feat/claude-code-plugin, not here. Real, and whichever lands second owns the regen; nothing I can do from this branch.
  • The five harness proposals (CE035–CE038, mccabe C90, diff-scoped mutation target, golden text artifact, pre-push hook) — all well-argued, and CE035 in particular would have caught blocker 1 statically. Each is a repo-wide gate with its own migration cost (C90 alone has 14 pre-existing offenders to grandfather), so they belong in their own PR rather than riding a criterion change. Not dismissing them.
  • YAML-surface coverage — the test file still constructs criteria in Python. Worth adding to test_success_criterion_union.py; noting rather than doing, since it's a broader test-strategy change than this PR.

Gate

  • tests/test_cli_called_criterion.py: 104 passed
  • Full suite: 4036 passed, 8 failed — all 8 verified identical on a clean main worktree at 57556af
  • make lint: 177 passed · ruff format --check and ruff check over the full LINT_PATHS: clean
  • pyright: the 3 pre-existing openai_codex import errors only

One correction to the review's blast-radius note: the out-of-tree consumer is real and I'm its author — the migrated IXP suite in UiPath/skills#2565. Nothing there uses the list form yet (it was written against verb as a string), so this reshaping breaks no existing task. The verb_any_of adoption happens there after a release, replacing the interim projects delete guard that exists precisely because alternation wasn't expressible.

@alexandrujircan alexandrujircan changed the title feat(cli-called): accept a list of verb spellings feat(cli-called): pin the verb and the argument tail (verb_any_of, exact_positional) Aug 12, 2026
@alexandrujircan alexandrujircan changed the title feat(cli-called): pin the verb and the argument tail (verb_any_of, exact_positional) feat(cli-called): accept alternative verbs via verb_any_of Aug 12, 2026
@alexandrujircan

Copy link
Copy Markdown
Contributor Author

Removed exact_positional in 8342c40 — it was answering a question about the matcher's semantics with a schema field before checking whether any assertion needed one.

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 cannot apply; the remaining 4 gain nothing real. Zero of 12 would have set it — while it charged the guard inversion plus the value_flags false-FAIL coupling you flagged. Documenting a hazard isn't the same as it being worth carrying, and the bug you found in its own positional: [] interaction was that cost arriving early. It's purely additive, so it can come back when something needs it.

Kept two pieces of that work, since neither depends on the field:

  • positional: [] is now rejected. Without exact_positional it is purely vacuous — it slices an empty expectation and compares it to itself — so it reads as "took no arguments" and asserts nothing. The at-least-one-facet check returns to plain falsiness, which is what catches verb: "".
  • The whitespace-normalization test, which pins a rendering change affecting every existing single-verb config.

Trailing arguments are unconstrained again, now a stated property of positional with a test pinning it rather than an accident.

verb_any_of stays — it fixes a live hole rather than a hypothetical one. PR title and description updated to one idea; both had been rewritten around the removed field.

Gate: 95 in the criterion file, 4027 full suite (same 8 pre-existing), lint 177, format/check clean.

alexandrujircan and others added 7 commits August 12, 2026 14:15
`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>
@alexandrujircan
alexandrujircan force-pushed the feat/cli-called-verb-alternation branch from c34f324 to d1b5974 Compare August 12, 2026 11:16
alexandrujircan and others added 2 commits August 12, 2026 14:20
…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>
Comment thread docs/TASK_DEFINITION_GUIDE.md Outdated
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants