feat(criteria): add cli_called for structured invocation matching - #72
Conversation
|
Claude Code is working… I'll analyze this and get back to you. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Claude finished @alexandrujircan's task in 1m 22s —— View job Code Review in Progress
|
b3a0c0d to
04dbb05
Compare
uipreliga
left a comment
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:72
Scope: pr:72 · branch feat/cli-called-criterion · 04dbb05 · 2026-08-03T22:01Z · workflow variant
Change class: complex — introduces a new success-criterion type with its own argv tokenization heuristics, flag-predicate matching, count-bound semantics and two model validators; correctness requires reasoning about parsing edge cases and scoring behavior
The PR's core machinery is healthy — clean architecture, strong typing discipline, near-total test and security hygiene (Architecture 9.8, Security 9.8, Test Health 9.5) — but the new cli_called criterion concentrates every real risk in one place: five confirmed defects (a boolean-switch parse bug, an ignore_flags/flags: precedence conflict, silently-dropped malformed records, an empty-verb vacuous config, and a lost --flag=-value binding) can each flip a task's score for identical agent output, almost always in the false-pass direction on exactly the max_count: 0 negative guards the feature was designed for, so the bottom line is: do not ship cli_called for authoring until the parse/precedence/degradation trio is fixed, and the rest of the PR is fine as-is.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 9.3 / 10 | 0 | 0 | 1 | 2 | _split_flags loses the value of an unambiguous --flag=-value and injects a spurious flag; two-pass tokenizer duplicates the --/end-of-flags state that causes it |
| 2. Type Safety | 8.4 / 10 | 0 | 1 | 1 | 1 | _validate_bounds accepts vacuous cli_called configs: empty/whitespace verb matches every record (1.0), and a dash-prefixed flag name makes absent: true always pass |
| 3. Test Health | 9.5 / 10 | 0 | 0 | 1 | 0 | No test for the most common negative shape: a required flag predicate against an invocation that omits the flag entirely |
| 4. Security | 9.8 / 10 | 0 | 0 | 0 | 2 | New file-read path bypasses the sandbox containment helper: cli_called.log is not constrained to the sandbox root (absolute paths, .., agent-planted symlinks all read outside it) |
| 5. Architecture & Design | 9.8 / 10 | 0 | 0 | 0 | 2 | min_count/max_count fields, the bounds validator, and the regex flags: int field are verbatim copies of existing criteria (with divergent scoring semantics) |
| 6. Error Handling & Resilience | 8.9 / 10 | 0 | 1 | 0 | 1 | Unparseable log lines and non-list[str] argv records are dropped without touching the score, so a max_count: 0 guard passes vacuously (contradicting the fail-loud missing-log path) |
| 7. API Surface & Maintainability | 7.9 / 10 | 0 | 1 | 2 | 1 | ignore_flags default ["output"] silently defeats an explicit flags: predicate on the same name (absent passes vacuously, equals can never match) — and its one real effect is untested |
| 8. Evaluation Harness Quality | 8.9 / 10 | 0 | 1 | 0 | 1 | Boolean switch before a positional silently swallows it — max_count: 0 guard passes on the forbidden invocation, and the positive form of the same assertion fails |
Overall Score: 9.1 / 10 · Weakest Axis: API Surface & Maintainability at 7.9 / 10
Totals: 🔴 0 · 🟠 4 · 🟡 5 · 🔵 10 across 8 axes.
Blockers
- [Axis 2]
_validate_boundsaccepts vacuous cli_called configs: empty/whitespaceverbmatches every record (1.0), and a dash-prefixed flag name makesabsent: truealways pass (src/coder_eval/models/criteria.py:445) — The guard at line 445 isif self.verb is None and not self.positional and not self.flags and self.tool is None:— note the asymmetry:positional/flagsare tested for falsiness, butverbonly foris None. Soverb: ""(or" ", or a${row.verb}dataset substitution that resolves to empty) passes validation, and incriteria/cli_called.py:136positional[: len(verb_tokens)] != verb_tokensdegenerates to[] != []→ every record matches. Verified against PR HEAD:CliCalledCriterion(description='d', log='mocks/calls.jsonl', verb='')is accepted and scores 1.0 on a log containing an unrelatedixp fields deletecall (same forverb=' '). A malformed criterion therefore reports a silent pass instead of failing. Fix: declare the field asverb: str | None = Field(default=None, min_length=1, ...)(line 402) plus a strip-and-reject check, and make the facet guard falsiness-symmetric (if not self.verb and not self.positional and not self.flags and not self.tool). Same class of hole on line 312:any_of: list[str] | Noneacceptsany_of: [], which passes_exactly_one_predicateand then can never match (allowed = set()incli_called.py:111), so amax_count: 0guard built on it passes vacuously — addmin_length=1there too. Both are AST-detectable as a candidate CE rule: a user-facingstr/listcriterion field consulted by anis Noneguard while siblings use falsiness. - [Axis 6] Unparseable log lines and non-list[str]
argvrecords are dropped without touching the score, so amax_count: 0guard passes vacuously (contradicting the fail-loud missing-log path) (src/coder_eval/criteria/cli_called.py:217) — Two degraded paths swallow bad input and never touch the score:
(1) cli_called.py:215-223 — try: parsed = json.loads(stripped) / except ValueError: malformed += 1; continue, plus the else: malformed += 1 for a non-dict line. malformed is used ONLY at line 257 (if malformed: details += f". Skipped {malformed} unparseable log line(s)"); the score at line 230 (score = 1.0 if within_lower and within_upper else 0.0) is computed purely from count, and error stays None.
(2) cli_called.py:124-126 — argv = record.get("argv") / if not isinstance(argv, list) or not all(isinstance(item, str) for item in argv): return False. This is indistinguishable from "did not match" and is not even counted in malformed (the record IS a dict, so it lands in records).
Verified by running the checker at PR HEAD against a real tempdir sandbox:
- log = one valid
getline + one truncated{"tool": "uip", "argv": ["ixp", "fields", "delete"...line, criterionverb: ixp fields delete, min_count: 0, max_count: 0→score=1.0, error=None, details='0 invocation(s) matched (verb=...); satisfies min_count=0, max_count=0. Skipped 1 unparseable log line(s)'. The forbidden call happened and the guard passed. - log =
{"tool": "uip", "argv": "ixp fields delete proj-1"}(mock recorded argv as a string, or used a different key name) →score=1.0, error=None, and the details string carries no hint at all.
This directly contradicts the decision documented 15-20 lines earlier at cli_called.py:195-205 ("A missing log is a harness fault, not agent behaviour... Failing... is what stops a negative guard — max_count: 0 — from passing vacuously against a log that does not exist") and the same reasoning in the docs (docs/TASK_DEFINITION_GUIDE.md, "A missing log file fails rather than counting as zero matches"). It also diverges from the sibling precedent: criteria/json_check.py:81-88 turns an unparseable payload into score=0.0, error=f"Invalid JSON in '{criterion.path}': {e}".
Fix: treat an unparseable line and a record whose argv is not list[str] as a harness fault on the same footing as a missing log — return score=0.0 with error=f"Invocation log '{criterion.log}' has N unusable record(s)" (optionally only when it could change the verdict, i.e. always for max_count is not None), and count the non-list-argv case into malformed so it is at least visible. tests/test_cli_called_criterion.py:308 pins only the positive direction (test_malformed_lines_are_skipped_and_reported, score 1.0); add the inverse case (malformed line + max_count: 0) per the "check the inverse of every new guard" technique.
3. [Axis 7] ignore_flags default ["output"] silently defeats an explicit flags: predicate on the same name (absent passes vacuously, equals can never match) — and its one real effect is untested (src/coder_eval/models/criteria.py:431) — ignore_flags: list[str] = Field(default_factory=lambda: ["output"], …) (criteria.py:431) is consumed unconditionally at positional, flags = _split_flags(argv, frozenset(criterion.ignore_flags)) (criteria/cli_called.py:128), and _split_flags drops the flag before any predicate runs (if name not in ignore: — cli_called.py:75). An explicitly authored predicate on a flag whose name is in ignore_flags is therefore silently ignored and mis-scored. Verified against a log line {"tool":"uip","argv":["ixp","projects","get","p1","--output","json"]}: flags: {output: {absent: true}} scores 1.0 ("1 invocation(s) matched … satisfies min_count=1") even though --output json WAS passed — a false pass on a negative guard; and flags: {output: "json"} scores 0.0 and can never pass. Neither path emits a warning, and the docs (docs/TASK_DEFINITION_GUIDE.md:780 ignore_flags: ["output"] # Flags dropped before matching) do not mention the interaction. Fix: in _validate_bounds (criteria.py:440) reject any key present in both flags and ignore_flags with a message naming the flag ("flag 'output' is both matched and in ignore_flags; remove it from ignore_flags or drop the predicate"), or subtract flags.keys() from the effective ignore set so an explicit predicate always wins. Add a test — grep -n ignore_flags tests/test_cli_called_criterion.py returns nothing, so the default is currently untested.
4. [Axis 8] Boolean switch before a positional silently swallows it — max_count: 0 guard passes on the forbidden invocation, and the positive form of the same assertion fails (src/coder_eval/criteria/cli_called.py:67) — _split_flags lines 67-78 assign the next token as a flag's value unless it starts with -: if index + 1 < len(tokens): candidate = tokens[index + 1]; if not candidate.startswith("-") or candidate == "-": flag_value = candidate; index += 1. So a boolean switch immediately preceding a positional eats it. Verified end-to-end against a real tempdir Sandbox: log line {"tool":"uip","argv":["ixp","fields","delete","--yes","proj-1"],"exit":0} with criterion verb: ixp fields delete, positional: [proj-1], min_count: 0, max_count: 0 returns score 1.0 — 0 invocation(s) matched (verb='ixp fields delete', positional=['proj-1']); satisfies min_count=0, max_count=0 — i.e. "did not delete proj-1" PASSES while the log proves the delete happened; the positive form (same criterion, default counts) returns 0.0. _split_flags(['ixp','fields','delete','--yes','proj-1'], frozenset(['output'])) → (['ixp','fields','delete'], {'yes': ['proj-1']}). The --yes/--force/--no-input-before-target shape is exactly how destructive CLIs are invoked, so this is the shape a negative guard most needs to catch. The mirror case is also wrong and undocumented: _split_flags(['ixp','proj','get','--limit','-1','proj-1']) → (['ixp','proj','get'], {'limit': [''], '1': ['proj-1']}) — the dash-leading value becomes a phantom flag named 1 (name = token.lstrip("-"), line 68) that consumes the following positional. Fix: make the value/switch decision declarable per flag instead of guessed — e.g. a boolean_flags: list[str] field on CliCalledCriterion (and/or treat --flag=value as the only value-bearing form for names the criterion doesn't mention), and add tests for --yes proj-1 and --limit -1 proj-1. tests/test_cli_called_criterion.py:343 test_boolean_switch_does_not_consume_the_next_flag only covers a switch followed by another flag, which is why this slipped.
Non-blocking, but please consider before merge
- [Axis 1]
_split_flagsloses the value of an unambiguous--flag=-valueand injects a spurious flag; two-pass tokenizer duplicates the--/end-of-flags state that causes it (src/coder_eval/criteria/cli_called.py:52) — Pass 1 flattens the equals form (line 52-55:if raw.startswith("-") and "=" in raw: name, _, value = raw.partition("="); tokens.append(name); tokens.append(value)), then pass 2 re-applies the "value must not start with-" heuristic (line 71-74:if not candidate.startswith("-") or candidate == "-": flag_value = candidate). For the equals form the binding is unambiguous, so re-applying the heuristic destroys information. Verified at PR HEAD:_split_flags(['get','--offset=-1'], frozenset())→(['get'], {'offset': [''], '1': ['']})— the value-1is dropped AND a bogus flag1is invented;_split_flags(['get','--exclude=-foo'], frozenset())→{'exclude': [''], 'foo': ['']}. Soflags: {offset: {equals: "-1"}}can never match--offset=-1, and the invented flag name can satisfy an unrelatedabsent/equalspredicate. The docstring's claim (line 25-26: "--flag=valueis split into--flag value, so the equals-form and the space-form compare equal") is honored only by making both wrong. Fix: bind the equals-form value directly in a single pass (flags.setdefault(name, []).append(value)), which also removes the duplicatedend_of_flagsstate machine (raw == "--"at line 48-51 andtoken == "--" and not end_of_flagsat line 63-66) and ~15 lines from the CC-17 function. Add a test alongsidetest_equals_form_and_space_form_are_equivalent(tests/test_cli_called_criterion.py:326) covering a dash-leading value. - [Axis 2] CliCalledCriterion's negative-guard docstring example is unconstructible:
max_count: 0withoutmin_count: 0is rejected by_validate_bounds(src/coder_eval/models/criteria.py:397) — The docstring block at lines 388-397 readsExample YAML (negative — must NOT have been called; ``max_count: 0``)::…max_count: 0with nomin_count, butmin_countdefaults to1(line 425) and_validate_bounds(line 441) raises. Verified against PR HEAD: constructing exactly that example yieldsValidationError: Value error, max_count (0) must be >= min_count (1). The parallel model 25 lines below gets it right — line 657 readsExample YAML (negative — must NOT run; uses ``min_count: 0`` + ``max_count: 0``):and itsmin_countdescription (lines 683-691) says "combine withmax_count: 0to expressmust NOT match".docs/TASK_DEFINITION_GUIDE.mdis also correct ("Setmin_count: 0andmax_count: 0"), so only the model docstring is wrong — and it is the copy-paste source a task author is most likely to reach for. Fix: addmin_count: 0to the example at line 397, retitle the heading at 388, and mirror the sibling'smin_countdescription on line 425 so the combination is documented on the field itself. - [Axis 3] No test for the most common negative shape: a required flag predicate against an invocation that omits the flag entirely (
src/coder_eval/criteria/cli_called.py:105) —_flag_matcheslines 104-105 are the "criterion requires a flag value, but the flag was never passed" path:
if values is None:
return FalseCoverage confirms it never executes (missed line 105; missing branch arc [104, 105]). Every negative flag test passes the flag with a wrong value instead — test_wrong_flag_value_does_not_match (tests/test_cli_called_criterion.py:64) records --model gemini_2_5_flash, and all eight test_predicate_forms rows (tests/test_cli_called_criterion.py:143) record --val <value>. Nothing exercises argv that lacks the flag.
This is the primary real-world miss for a positive cli_called criterion ("did the agent pass --model at all?"). I verified today's behaviour is correct (verb: ixp projects configure-model, flags: {model: "pro"} vs argv: ["ixp","projects","configure-model","proj-1"] → score 0.0), so this is a coverage gap, not a bug — but it is the branch that separates equals from absent, and it is unpinned. Add a missing-flag test for at least equals asserting score 0.0.
4. [Axis 7] Bare-scalar flags: shorthand coerces only str, so unquoted YAML numeric/boolean flag values fail with an opaque pydantic model_type error (src/coder_eval/models/criteria.py:325) — _coerce_scalar_shorthand (criteria.py:325-329) coerces only isinstance(value, str), but the advertised contract is unconditional: "In YAML a bare scalar is accepted as shorthand for equals" (criteria.py:289-291), "A bare scalar means 'equals'" (criteria.py:421), and model: "gemini_2_5_pro" # Bare scalar == {equals: ...} (docs/TASK_DEFINITION_GUIDE.md:776). YAML scalars that are not strings therefore fail: flags: {retries: 3} → flags.retries: Input should be a valid dictionary or instance of FlagMatch [type=model_type, input_value=3, input_type=int], and flags: {verbose: true} → same error with input_type=bool (both verified). Numeric flag values (--retries 3, --top-k 5) are a common CLI case and argv values are always strings anyway. Fix: coerce str | int | float | bool to {"equals": str(value)} (mapping bools to the CLI's spelling, or rejecting bool with an explicit message that points at the switch-presence form), and add a validation test for the unquoted-number case.
5. [Axis 7] Doc surfaces not updated for the 15th criterion type: stale "14 criterion types" counts in the task guide, and cli_called missing from CLAUDE.md's table/tree and the task-create command's tables (src/coder_eval/models/criteria.py:1151) — | CliCalledCriterion was added to the SuccessCriterion union (criteria.py:1151) and to docs/TASK_DEFINITION_GUIDE.md, but the other enumerations of criterion types were not updated: CLAUDE.md:37 (criteria.py # 14 success criterion types + base + union), CLAUDE.md:147 (## Success Criteria (14 types)) and its table (14 rows, no cli_called — CLAUDE.md:158 is command_executed, CLAUDE.md:162 is skill_triggered), and .claude/commands/coder-eval-task-create.md — both its selection table (line 60/62) and its per-type field table (lines 165-178, which ends at uipath_eval). That last one is the task-authoring surface, so the generator that writes task YAML cannot emit cli_called. Fix: bump both CLAUDE.md counts to 15 and add the table row + the two rows in coder-eval-task-create.md. This is mechanically enforceable and nothing guards it today (tests/lint/rules/ has no criterion-enumeration rule): propose a CE032 doc-surface rule in the CE027–CE031 family asserting every member of the SuccessCriterion union appears as inline code in CLAUDE.md's criteria table and in the guide's TOC.
Nits
- [Axis 1]
_flag_matchesends in an unreachablereturn False(dead branch), and_compiled's lru_cache duplicatesre's own pattern cache (src/coder_eval/criteria/cli_called.py:85) —@lru_cache(maxsize=256)+def _compiled(pattern, flags)(lines 85-93) carries a 6-line docstring justifying itself ("A log can hold hundreds of invocations; recompiling the same pattern for each is pure waste"), butre.compilealready memoizes: verified at PR HEAD,re._MAXCACHE == 512and twore.compile('abc')calls leavelen(re._cache) == 1. The sibling checkers callre.compiledirectly (command_executed.py:196,file_matches_regex.py), so this is an extra abstraction with no measurable benefit and one drawback — a permanently-retained module-level cache of task-supplied patterns. Drop_compiledand callre.compile(predicate.matches_regex, predicate.flags)at both sites (lines 114 and 186). Separately,_flag_matches's trailingreturn False(line 116) is unreachable:FlagMatch._exactly_one_predicate(models/criteria.py:332) guarantees exactly one predicate is set, so every path returns earlier — either delete it or make itraise AssertionErrorso a future predicate added without a matcher arm fails loudly instead of silently scoring 0.0. - [Axis 1]
FlagMatchships a 5-way predicate union with 4 validation rules for a criterion no in-tree task uses yet (src/coder_eval/models/criteria.py:307) —contains(line 307),any_of(line 312) andflags: int(line 314) are each expressible with thematches_regexpredicate the same model already has (any_of: [a, b]≡matches_regex: '^(a|b)$'), and the 5-way union then needs four policing rules to stay coherent:_coerce_scalar_shorthand(line 325), the exactly-one check (line 332), the "flagsonly withmatches_regex" cross-field check (lines 345-348), and the 5-armifchain incriteria/cli_called.py:102-116.grep -rn cli_calledover the PR HEAD tree returns hits only indocs/TASK_DEFINITION_GUIDE.md,src/, andtests/— notasks/*.yamlconsumes the criterion, so the extra predicates are speculative surface rather than extracted need. Consider shippingequals/absent/matches_regex(+flags) and addingcontains/any_ofwhen a real task asks; if they stay, at least dropany_of, whose only advantage over a regex is avoiding escaping. Note also thatflags: intexposes rawre-module integers to YAML authors (flags: 16for DOTALL) — accepted here for parity withFileMatchesRegexCriterion.flags(line 283), so parity, not readability, is the argument. - [Axis 2]
FlagMatch.flagsis an unconstrainedint(notre.RegexFlag) and its name collides withCliCalledCriterion.flags; the up-front compile guard only catchesre.error, so a bad flags value escapes it (src/coder_eval/models/criteria.py:314) — Line 314 declaresflags: int = Field(default=0, ...)with no bound, so any integer is accepted and only surfaces atre.compiletime. Verified: an out-of-range bit raisesValueError(re.compile('a', 99999999)→ValueError: cannot use LOCALE flag with a str pattern), andre.erroris not aValueErrorsubclass, so the deliberate pre-flight guard incriteria/cli_called.py:187(except re.error as exc:→error=f"Invalid matches_regex for flag '{name}': {exc}") does not fire; the failure falls through to the generichandle_criterion_errorscapture and loses the offending flag name — defeating the stated intent of the comment on lines 179-181. Secondarily,FlagMatch.flags: int(regex flags) andCliCalledCriterion.flags: dict[str, FlagMatch](predicate map, line 418) share a name and are read two lines apart incli_called.py:182-186(criterion.flagsvspredicate.flags) — a type-confusion trap for the next editor. Fix: type itre.RegexFlag(Pydantic validates IntFlag membership) or addge=0+ a validator, broaden line 187 toexcept (re.error, ValueError), and consider naming itregex_flags. Note thisinttyping is exact parity with the pre-existingFileMatchesRegexCriterion.flags(line 458) and its checker's identicalexcept re.error, so this is convention drift rather than a novel mistake — worth a small shared helper rather than a third copy. - [Axis 4] New file-read path bypasses the sandbox containment helper:
cli_called.logis not constrained to the sandbox root (absolute paths,.., agent-planted symlinks all read outside it) (src/coder_eval/criteria/cli_called.py:195) —cli_called.py:195callsif not sandbox.file_exists(criterion.log):and:207content = sandbox.get_file_content(criterion.log). Both sandbox methods do a bare join with no containment check —sandbox.py:1001return (self.sandbox_dir / path).exists()andsandbox.py:986-987file_path = self.sandbox_dir / path/return file_path.read_text(encoding="utf-8"). Because pathlib's/discards the left operand for an absolute right operand,log: "/etc/passwd"(orlog: "../../.env") reads straight off the host, contradicting the field's own promise atmodels/criteria.py:401:log: str = Field(description="Path to the JSON Lines invocation log, relative to the sandbox working directory"). Two aggravating notes: (a) the repo already has the right primitive —Sandbox._resolve_within_sandbox(rel, field=...)atsandbox.py:295-300(if candidate != sandbox_root and sandbox_root not in candidate.parents: raise RuntimeError(f"{field} escapes sandbox: ...")), used formount_pointandmock_path_dirswith the explicit rationale atsandbox.py:439-441that "a typo like../mockswould otherwise let_prepare_mock_path_dirschmod +x files on the host filesystem" — the new criterion does not reach for it; (b)logis a string leaf ofsuccess_criteria, sotask_loader${row.<field>}substitution (task_loader.py:331"Walk a nested dict/list structure and substitute ${row.X} in every string leaf") can inject dataset values into it, widening the path's provenance beyond the hand-written YAML. Severity is held at Low because task YAML is operator-authored (PR:H) and no log content is echoed intodetails/error(the strings built atcli_called.py:247-258contain only counts and criterion config), so there is no disclosure sink; the residual real trust-boundary case is the agent — which controls sandbox contents — plantingmocks/calls.jsonlas a symlink to an evaluator-readable file, sinceread_textfollows symlinks. Fix: resolvecriterion.logthrough_resolve_within_sandbox(promote it to a publicSandboxAPI), whose.resolve()also collapses symlinks and would reject both the escape and the symlink variant. CVSS:3.1/AV:L/AC:L/PR:H/UI:N/S:U/C:L/I:N/A:N - [Axis 4] Agent-controlled invocation log is parsed with no resource bounds — whole-file read plus per-value regex backtracking on a
to_threadworker that survives thetask_timeoutcancel (src/coder_eval/criteria/cli_called.py:207) — The log is agent-influenced (the shadowed mock records the agent's own argv, and the agent has shell in the sandbox and can append tomocks/calls.jsonldirectly), yet it is processed with no size or time cap. Two compounding sites: (1)cli_called.py:207content = sandbox.get_file_content(criterion.log)slurps the whole file, and:209-223then materializes every line as a dict inrecords: list[dict[str, Any]] = []— a large log is resident twice (raw string + parsed dicts), so an oversized log OOMs the evaluator process rather than the sandbox; (2)cli_called.py:114-115regex = _compiled(predicate.matches_regex, predicate.flags)/return any(regex.search(value) is not None for value in values)runs an operator-authored pattern against agent-controlled flag values once per value per record, so a backtracking-prone pattern ((a+)+) plus a crafted value is a ReDoS. The blast radius is worse than a normal hang because_check_implis offloaded atcriteria/base.py:403-410viareturn await asyncio.to_thread(self._check_impl, ...): theThreadedWatchdogatorchestrator.py:489-493cancels the asyncio task, but the non-daemon default-executor thread keeps spinning and is joined at interpreter exit, so the harness process wedges pasttask_timeout. This is parity with the existing whole-file criteria (file_contains.py:45,file_matches_regex.py:51,json_check.py:81— none of which cap size either), which is why it is Low rather than higher, but this is a new intake point for untrusted content. Fix: stat-and-cap the log before reading (or streamsplitlinesfrom an opened handle) and cap the record count, and document the ReDoS exposure ofFlagMatch.matches_regex(models/criteria.py:308-313) since the value it is applied to is agent-supplied. CVSS:3.1/AV:L/AC:L/PR:L/UI:N/S:U/C:N/I:N/A:L - [Axis 5] min_count/max_count fields, the bounds validator, and the regex flags: int field are verbatim copies of existing criteria (with divergent scoring semantics) (
src/coder_eval/models/criteria.py:425) — Three copy-paste sites withinmodels/criteria.py: (a)min_count/max_count(lines 425-436) duplicateCommandExecutedCriterion.min_count/max_count(lines 683-700); (b)_validate_bounds(line 441) is a verbatim clone of_validate_count_bounds(line 710), down to the message —f"max_count ({self.max_count}) must be >= min_count ({self.min_count})"; (c)flags: int = Field(default=0, …)(line 314) is the third copy of the raw-re-integer knob afterFileMatchesRegexCriterion.flags(line 283) andRegexPattern.flags(line 458). Worth noting the (a) copy also diverges in meaning: identically-named fields score fractionally oncommand_executed(min(1.0, match_count / criterion.min_count), criteria/command_executed.py:241) but binary oncli_called(score = 1.0 if within_lower and within_upper else 0.0, criteria/cli_called.py:230), somin_count: 3with 1 match yields 0.33 on one criterion and 0.0 on the other. Fix: extract a sharedCountBoundsMixin(fields + validator) and aRegexFlagsannotated alias, and state the binary-vs-fractional difference in both field descriptions rather than only in the checker docstring. - [Axis 5] Internal product vocabulary (ixp / uip / labellings / project + model ids) embedded in a going-public core model's docstring and examples (
src/coder_eval/models/criteria.py:363) — The new model's record-schema and YAML examples are written entirely in internal UiPath product vocabulary:{"argv": ["ixp", "projects", "get", "proj-1", "--output", "json"], "tool": "uip", …}(line 363),verb: "ixp projects configure-model"(line 382),positional: ["my_invoices-f1afa9ef-ixp"](line 383),verb: "ixp labellings confirm"(line 394), and the field description"…(e.g. 'ixp projects configure-model')"(line 406).grep -rni ixp src/coder_eval/returns hits only in these new lines (the pre-existinguipreferences live in sandbox/utils/orchestrator version-capture, which is a different, already-established case). This is docstring-only — no import or behavior couples the core to an internal system, so the wheel stays strippable — but for a core that is going OSS-public, the vendor-neutral illustration (git,docker,terraform) reads better and keeps the vendor flavor intasks/. Fix: reword the docstring/field examples with a generic CLI; the UiPath-shaped example can live in docs/TASK_DEFINITION_GUIDE.md or a task YAML. - [Axis 6] Every degraded path in the new checker is silent —
loggeris bound at module scope and never called (src/coder_eval/criteria/cli_called.py:17) —logger = logging.getLogger(__name__)at line 17 is never used anywhere in the 265-line module (grep -c 'logger\.' src/coder_eval/criteria/cli_called.py→ 0). Unlike its siblings, this checker has genuinely degraded paths worth a log record: unparseable lines (cli_called.py:217-219), non-dict lines (:223), and records rejected for a non-list[str]argv(:125). Today the only trace of any of them is thedetailsstring appended at:257-258, and theargvcase leaves no trace at all — so a task author whose mock writes a slightly different record schema has nothing intask.logto diagnose from. Emitlogger.warning(...)namingcriterion.logand the dropped-line count on those paths (this is the observability half of finding #1; the scoring half is the fix at:217). - [Axis 7] Path field named
logwhile all six sibling file-reading criteria name itpath(src/coder_eval/models/criteria.py:401) — criteria.py:401 islog: str = Field(description="Path to the JSON Lines invocation log, relative to the sandbox working directory"), whereas every other sandbox-file criterion usespath: criteria.py:197, 207, 280, 544, 576, 764 (e.g.path: str = Field(description="Path to the file to check")). A task author who has internalizedpath:will mistype it and get anextra_forbiddenerror on a new key rather than a hint. Either rename topath(greenfield — no compat cost) or keeplogand state the deviation in the guide'scli_calledsection so it reads as deliberate. - [Axis 8] No predicate expresses "flag was passed" (the inverse of
absent), so asserting a boolean switch needs the undocumentedequals: ""(src/coder_eval/models/criteria.py:313) —FlagMatchoffersequals/contains/matches_regex/any_of/absent: bool = Field(default=False, description="Flag must NOT be present in the invocation")(criteria.py:306-313) — there is nopresent. Boolean switches are recorded with value""(flags.setdefault(name, []).append(flag_value if flag_value is not None else ""), cli_called.py:76), so "the agent passed--force" must be written as the non-obviousequals: ""(which then breaks if the mock records--force true) orcontains: ""(which matches any value); neither is documented in the predicate table at docs/TASK_DEFINITION_GUIDE.md:789-797. The natural attemptabsent: falseis rejected withFlagMatch requires exactly one of equals / contains / matches_regex / any_of / absent, got none, which does not hint at the right spelling. Add apresent: boolpredicate (or documentcontains: ""as the presence idiom in the predicate table).
What's Missing
Parallel paths:
- 🟠 The producer half of the new contract is missing: nothing in-tree writes the JSONL invocation log the criterion consumes.
grep -rl calls.jsonlhits onlysrc/coder_eval/models/criteria.py,src/coder_eval/criteria/cli_called.py's docs,docs/TASK_DEFINITION_GUIDE.mdand the test file — no recording-mock shim, notemplates/entry, no task YAML. The repo already has the exact pattern to extend (tasks/mock_path_dirs_smoke.yaml+sandbox.mock_path_dirs+ astarter_filesshim, cf.tasks/mock_path_dirs_template_dir/mock-cli-bins/mocks/echo_args), and the guide's cli_called section never mentionsmock_path_dirs, so a task author has no path from "shadow a CLI" to "get a log". Add a recording-mock shim + acli_calledsmoke task so the record schema has one in-repo writer. (trigger: src/coder_eval/criteria/cli_called.py) - 🟡
command_executed— the criterion cli_called is explicitly positioned to replace ("Use this instead ofcommand_executedorfile_matches_regex…", guide:767) — was not touched: no back-reference from its own guide section or docstring, no shared count-bounds mixin, and itsmin_countscores fractionally (criteria/command_executed.py:241) where cli_called's identically-named field scores binary (criteria/cli_called.py:230). That divergence also propagates into the per-type rollupaverage_score(reports.py:788,:928) with no note indocs/REPORT_SCHEMA.md. (trigger: src/coder_eval/models/criteria.py) (restates: Axis 5: min_count/max_count fields, the bounds validator, and the regex flags: int field are verbatim copies of existing criteria) - 🔵
cli_calleddeclares nolive_stop_polarities/live_verdict, so unlikecommand_executedandskill_triggeredit can never armrun_limits.stop_early. A task migrating a "did the agent call X" assertion fromcommand_executedtocli_calledwhile usingstop_earlyhits a hard error at resolution (orchestration/early_stop.py:190). The choice is defensible for a file-backed criterion, but it is unstated in both the model docstring and the guide'sstop_earlysection. (trigger: src/coder_eval/criteria/cli_called.py) - 🔵 The new sandbox file read does not reach for the containment primitive the sandbox layer already uses for
mount_pointandmock_path_dirs(Sandbox._resolve_within_sandbox, sandbox.py:295-300), sologis joined bare like the older file criteria rather than following the newer, safer pattern. (trigger: src/coder_eval/criteria/cli_called.py) (restates: Axis 4: New file-read path bypasses the sandbox containment helper)
Tests:
- 🟠
positionalalone is accepted as the sole facet by_validate_bounds, but withverbunsetoffsetstays 0, sopositionalis anchored atargv[0]: verifiedCliCalledCriterion(positional=["proj-1"])returns False againstargv=["ixp","projects","get","proj-1"]and True only againstargv=["proj-1"]. The guide describespositionalas "Non-flag arguments following the verb", so this validated-but-silently-always-failing shape is both undocumented and untested (every positional test intests/test_cli_called_criterion.pypairs it with averb). Add a positional-without-verb test pinning the intended semantics, or requireverbwheneverpositionalis set. (trigger: src/coder_eval/models/criteria.py) (restates: Axis 2:_validate_boundsaccepts vacuous cli_called configs) - 🟠 No inverse test for the degraded-input guards:
test_malformed_lines_are_skipped_and_reported(line 308) pins only the score-1.0 direction, and nothing covers malformed-line-plus-max_count: 0or a record whoseargvis notlist[str]. Per the "check the inverse of every new guard" technique, both cases need a test asserting the guard cannot pass vacuously. (trigger: tests/test_cli_called_criterion.py) (restates: Axis 6: Unparseable log lines and non-list[str]argvrecords are dropped without touching the score) - 🟡 Short-flag forms are neither tested nor documented:
name = token.lstrip("-")(cli_called.py:68) makes-m prorecord the flag asm, soflags: {model: "pro"}silently scores 0.0 when the agent uses the short alias;_split_flags(["rm","-rf","/tmp/x"], frozenset())→{'rf': ['/tmp/x']}(cluster becomes one flag namedrfthat swallows the path); and the defaultignore_flags: ["output"]does not cover-o. Add tests for the short form and state in the guide that flag names are matched literally with no alias table (adjacent to the tokenizer heuristic in Axis 8). (trigger: src/coder_eval/criteria/cli_called.py) - 🟡 The "criterion requires a flag value but the flag was never passed" branch (cli_called.py:104-105) never executes under the suite — the primary real-world negative shape for a positive
cli_calledcriterion. Confirmed coverage: 95.50%, missing lines 105, 116, 126, 214, 223. (trigger: src/coder_eval/criteria/cli_called.py) (restates: Axis 3: No test for the most common negative shape: a required flag predicate against an invocation that omits the flag entirely) - 🟡 No test names
ignore_flagsat all (grep -n ignore_flags tests/test_cli_called_criterion.py→ nothing).test_output_is_ignored_by_default(line 328) covers only the benign case where the criterion does not mention--output; the case that actually mis-scores — aflags:predicate on a name that is also inignore_flags— is unpinned, as is a non-defaultignore_flagslist. (trigger: tests/test_cli_called_criterion.py) (restates: Axis 7: ignore_flags default ["output"] silently defeats an explicitflags:predicate on the same name) - 🔵 Two integration surfaces of the new type are untested: dataset fan-out (
${row.*}substitution intolog/verb/positional/ bare-scalarflags, the path where a null row value collapsesverbto""), and the inheritedaggregate()/suite_thresholdsgate for a binary criterion on a dataset-backed suite. All 38 new tests construct the criterion directly with literal values. (trigger: tests/test_cli_called_criterion.py)
Display & mapping dicts:
- 🟡 Beyond the CLAUDE.md / task-create surfaces already filed, three more "14 criterion types" enumerations were left stale by the 15th type — including one inside an in-scope file:
docs/TASK_DEFINITION_GUIDE.md:4(frontmatter description, "all 14 success criterion types"),docs/TASK_DEFINITION_GUIDE.md:527("The framework supports 14 criterion types"), anddocs/comparison.md:87("14 weighted criterion types"). CLAUDE.md'scriteria/file tree (lines 48-64) also omitscli_called.py. (trigger: docs/TASK_DEFINITION_GUIDE.md) (restates: Axis 7: Doc surfaces not updated for the 15th criterion type)
Daily/nightly:
- 🟡 Blast radius on the production path is unstated. No in-tree task uses
cli_called, so today's nightly is unaffected — but the motivating suite (ixp/uipverbs,mocks/calls.jsonl) and the recording mock live outside this repo, which makes the JSONL record shape a new cross-repo contract with no version marker, no shared writer, and (per the Axis 6 finding) silent tolerance of a wrong-shaped record. The PR should say which nightly suite adopts it, that the mock must land before any nightly task depends on it, and how mock/checker drift gets detected. (trigger: src/coder_eval/criteria/cli_called.py)
Downstream consumers:
- 🔵
exit/tsare documented as "recorded for reporting rather than matched" (guide:783; same claim in thecriteria.pyrecord-schema docstring), but no reporting consumer exists: neitherreports.py,reports_html.py,reports_junit.pynor the evalboard ever reads the invocation log, and the test helper_call(tests/test_cli_called_criterion.py:29) defaultsexit_code=1without any assertion noticing. Either drop the reporting claim or add an exit-status predicate — "the delete actually succeeded" is currently inexpressible. (trigger: docs/TASK_DEFINITION_GUIDE.md)
Harness & Lint Improvements
Static checks (lint / type):
- [ce-lint] CE032 — docstring YAML examples must construct. Extend the CE029 family (
tests/lint/doc_examples.py, wired astests/test_custom_lint.py::TestCE032DocstringYamlExamples) from Markdown to Python docstrings undersrc/coder_eval/models/: walk everyClassDefdocstring, lift each indented block following anExample YAML…::line,yaml.safe_loadit, and validate anysuccess_criteriarows throughTypeAdapter(SuccessCriterion)(same whole-document/fragment classification andlint-skipescape hatch CE029 already uses). I prototyped this against the PR worktree: it validates 8 example rows and reports exactly 1 invalid — thecli_callednegative block (Value error, max_count (0) must be >= min_count (1)) — with zero false positives. It additionally surfaces 4 pre-existing blocks that are not even valid YAML (command_pattern: "uip\s+or\s+users\s+list"/"curl.*api\.example\.com"— an unquoted-escape YAML error inside a double-quoted scalar, i.e. published examples that fail on copy-paste), so the rule pays for itself immediately. Prevents: The 4-axis-duplicated finding "CliCalledCriterion's negative-guard docstring example is unconstructible (max_count: 0withoutmin_count: 0)" (models/criteria.py:388-397), plus 4 latent copy-paste-broken examples in the same file. - [ce-lint] CE033 — criterion registry ↔ doc-surface parity. New whole-tree rule in the CE027–CE031 family (
tests/lint/doc_criteria_parity.py, wired as a@pytest.mark.linttest class): enumerate thetypeliteral of every member of theSuccessCriterionunion and assert each appears (a) as inline code in CLAUDE.md's Success Criteria table, (b) as a checker filename in CLAUDE.md'scriteria/tree listing, (c) indocs/TASK_DEFINITION_GUIDE.md, and (d) in both tables of.claude/commands/coder-eval-task-create.md; additionally assert the literal counts in CLAUDE.md (# 14 success criterion types…,## Success Criteria (14 types)) equal the union arity. Verified mechanically on the PR worktree: the union has 15 members and the check reports exactly['cli_called']missing from CLAUDE.md and from coder-eval-task-create.md, and nothing missing from the guide — i.e. it pins precisely the drift this PR introduced and stays green on everything else. Prevents: "Doc surfaces not updated for the 15th criterion type" (stale 14-type counts + missing table/tree rows in CLAUDE.md, missing rows in the task-create command that make the task generator unable to emitcli_called). - [ce-lint] CE034 — "at least one of" facet guards must be falsiness-symmetric. New AST rule (
tests/lint/rules/ce034_facet_guard_symmetry.py, registered inALL_RULES): inside a@model_validator(mode="after"), when aBoolOp(And)guards araisewhose message contains "at least one of" / "requires at least one", every operand must be the same shape —not self.<field>— and mixingself.<field> is Nonewithnot self.<other>is a violation; additionally require eachstr/listfield named in such a guard to declaremin_length=1(or aNonEmptyStr = Annotated[str, StringConstraints(min_length=1, strip_whitespace=True)]alias) in itsField(...).# noqa: CE034for a deliberate exception. Prevents: The high-severity "_validate_boundsaccepts vacuous cli_called configs" finding (models/criteria.py:445 —verb=""/verb=" "/tool=""slip theis Nonehalf of the guard and score 1.0 on unrelated invocations;any_of: []slips_exactly_one_predicateand can never match, so amax_count: 0guard built on it passes vacuously). - [ce-lint] CE039 — a dropped-record counter must reach the verdict, not just
details. New AST rule scoped tosrc/coder_eval/criteria/: if a local integer is incremented (x += 1) inside anexcepthandler or a degraded-inputelse/guard branch, that name must also appear inside thescore=orerror=keyword argument of a returnedCriterionResult— appearing only inside adetails=f-string is a violation (# noqa: CE039with a reason for the cases where dropping really is verdict-neutral). Sibling precedent for the shape: CE019 (TelemetryNonFatal). This is deliberately a narrow heuristic on one mechanical pattern — an unparseable-input counter that can never change the score. Prevents: The high-severity "unparseable log lines and non-list[str]argvrecords are dropped without touching the score, somax_count: 0passes vacuously" finding (cli_called.py:217-223 / 124-126, wheremalformedis read only at line 257 in thedetailsstring), and the divergence from thejson_check.py:80-90precedent. - [ce-lint] CE036 — criteria must read sandbox artifacts through a contained, size-capped helper. Add
Sandbox.read_task_file(path, *, field=…, max_bytes=…)that routes through the existingSandbox._resolve_within_sandboxcontainment check (sandbox.py:276, already used formount_point/mock_path_dirs/starter_files) and stats-and-caps before reading; then add an AST rule forbidding directsandbox.get_file_content(...)/sandbox.file_exists(...)calls insidesrc/coder_eval/criteria/. Note the deliberate carve-out documented at sandbox.py:965 (the raw accessors stay traversal-permissive for non-grading use) — CE036 constrains the grading path only, where the field's own contract says "relative to the sandbox working directory". Would also retro-fix the 6 pre-existing sibling call sites (file_contains,file_check,file_matches_regex,json_check,classification_match,uipath_eval). Prevents: "New file-read path bypasses the sandbox containment helper" (log: "/etc/passwd"/../../.env/ agent-planted symlink read off the host, cli_called.py:195/207) and the read-side half of "agent-controlled invocation log is parsed with no resource bounds". - [ce-lint] CE035 — exhaustive predicate dispatch. New AST rule: a function in
src/coder_eval/criteria/that dispatches over the mutually-exclusive predicate fields of a model guarded by an exactly-one-of validator (e.g.FlagMatch) must reference every such field of that model, and must terminate inraise AssertionError/assert_neverrather than a bare falsyreturn— so a predicate added to the model without a matcher arm fails the build instead of silently scoring 0.0. Pair the rule with the model side: the field set is read off the Pydantic model, so the two can't drift. Prevents: "_flag_matchesends in an unreachablereturn False(dead branch)" (cli_called.py:116) and the latent version of it — a futurepresent/new predicate added toFlagMatchthat silently grades every invocation as a non-match. - [ce-lint] Extend CE030's
DOCUMENTED_MODELSregistry to the criterion models a PR adds. One-line registry change intests/lint/doc_schema_parity.py: add(CliCalledCriterion, "docs/TASK_DEFINITION_GUIDE.md")and(FlagMatch, "docs/TASK_DEFINITION_GUIDE.md")(and make adding a union member to the registry part of the new-criterion checklist). Verified against the PR worktree: this fails today on exactlyCliCalledCriterion: ['log', 'positional', 'ignore_flags']andFlagMatch: ['absent']— i.e. it forces prose documentation of precisely the fields whose semantics the review found undocumented (theignore_flagsprecedence interaction, theabsentinverse / presence idiom, and thelog-vs-pathnaming deviation). Prevents: "ignore_flagsdefault silently defeats an explicitflags:predicate" (undocumented interaction), "No predicate expresses 'flag was passed'" (undocumentedequals: ""idiom), and "Path field namedlogwhile all six siblings name itpath" (forces the deviation to be stated). - [ce-lint] CE037 — shared aliases/mixins for the duplicated criterion knobs. Extract
CountBounds(themin_count/max_countfields + the singlemax_count >= min_countvalidator) andRegexFlags = Annotated[re.RegexFlag, …], then add an AST rule: a criterion model declaring bothmin_countandmax_countmust inheritCountBounds(no third hand-rolled copy of the identical validator and message), and a field namedflags/*_flagstyped as an integer regex-flag knob must use theRegexFlagsalias rather than bareint. Prevents: "min_count/max_count fields, the bounds validator, and the regexflags: intfield are verbatim copies of existing criteria" (models/criteria.py:314/425/441 vs 283/458/683-710) — and, because the mixin carries one docstring, the divergent binary-vs-fractional scoring semantics get documented in one place. - [ce-lint] CE040 —
except re.erroris too narrow where flags are externally supplied. AST rule scoped tosrc/coder_eval/criteria/: anexcept re.errorhandler wrapping are.compile(...)call whose flags argument comes from a criterion field must beexcept (re.error, ValueError).re.erroris not aValueErrorsubclass, andre.compile('a', 99999999)raisesValueError, so the deliberate flag-naming pre-flight guard at cli_called.py:187 never fires for a badflagsvalue and the error loses the offending flag name. Prevents: Theflags-half of "FlagMatch.flagsis an unconstrainedint… the up-front compile guard only catchesre.error, so a bad flags value escapes it" — plus the identical pre-existing shape infile_matches_regex. - [ce-lint] CE041 — a module that binds
loggermust use it. Trivial AST rule: a module-levellogger = logging.getLogger(__name__)with nologger.<level>(...)call anywhere in the module is a violation (ruff's F841 is function-scoped and does not reach module bindings). Honest cost note: this flags ~10 pre-existing modules today (criteria/cli_called.py,criteria/file_contains.py,criteria/json_check.py,agents/codex_agent.py, …), so it lands together with a mechanical cleanup pass — either delete the dead binding or add the log call the degraded path deserves. Prevents: "Every degraded path in the new checker is silent —loggeris bound at module scope and never called" (cli_called.py:17): today a mock writing a slightly different record schema leaves nothing at all intask.log. - [ruff] Enable
C901atlint.mccabe.max-complexity = 15, with aper-file-ignoresbaseline for the existing offenders — exactly the "gate NEW growth past these bounds" philosophy already documented inpyproject.tomlforPLR0915/PLR0912(whose 80-statement / 25-branch ceilings are too loose to bite here). Measured on the PR worktree: 15 violations at threshold 15 (28 at 12, 5 at 18), so a 15 baseline is 14 pre-existing files plus the newCliCalledChecker._check_implat complexity 17 — the function that bundles sandbox read + JSON-Lines parse + match loop + malformed-counting + details assembly into one body. Prevents: Pressures the two structural findings on the new checker: the_check_implgod-function that hosts the silent-drop path (cli_called.py:217) and the duplicatedend_of_flagsstate machine in the two-pass_split_flags(radon CC 17) whose second pass destroys the unambiguous--flag=valuebinding. - [pyright] Type regex-flag knobs as
re.RegexFlag, not bareint(flags: re.RegexFlag = Field(default=re.NoFlag, …)onFlagMatch,FileMatchesRegexCriterion,RegexPattern— theRegexFlagsalias from CE037 above). Pydantic then validates IntFlag membership at load time (rejectingflags: 99999999in YAML with a field-anchored message instead of a mid-checkValueError), and pyright rejects arbitrary-int construction sites in-tree under the existingstandardmode without any config change. Consider also renaming toregex_flagsso it stops colliding withCliCalledCriterion.flags(the predicate map) — the two are read two lines apart at cli_called.py:182-186. Prevents: "FlagMatch.flagsis an unconstrainedint(notre.RegexFlag) and its name collides withCliCalledCriterion.flags" — both the unvalidated-value half and the type-confusion-trap half. - [bandit-codeql] Add CodeQL
security-extendedforsrc/coder_eval/(queriespy/path-injection,py/polynomial-redos,py/regex-injection) — but record explicitly that it does NOT reach these two findings. Both the log path and the regex pattern originate in operator-authored task YAML, which no CodeQL taint source model recognizes, and bandit has no ReDoS check at all; the value of enabling the suite is future coverage of the agent-sourced variants (the sandbox contents the agent controls), not this PR. The actual gate for these two findings is CE036 (contained + capped read) plus the watchdog-escape test in the harness bucket. Prevents: Records the boundary for "cli_called.log is not constrained to the sandbox root" and "agent-controlled invocation log is parsed with no resource bounds (whole-file read + per-value regex backtracking)" — i.e. these are deliberately routed to CE036 and a runtime guard, not left uncovered by omission.
Harness improvements (not statically reachable):
- A negative-guard (vacuous-pass) fixture corpus for
cli_called, asserted in both directions. One parametrized table of real argv shapes, each row run twice against the same log — once as the positive criterion (must score 1.0) and once as themin_count: 0, max_count: 0guard (must score 0.0):["ixp","fields","delete","--yes","proj-1"](switch before positional),--force proj-1,--limit -1 proj-1(dash-leading value → phantom flag1),--offset=-1and--exclude=-foo(equals form with a dash-leading value),--output jsonwith an explicit predicate onoutput(theignore_flagsoverlap), a required flag simply absent from argv, a truncated JSON line, andargvrecorded as a string instead of a list. Today at least four of these rows fail — the delete guard scores 1.0 while the log proves the delete happened. Why not static: The defect is semantic argv-grammar ambiguity plus end-to-end scoring: no AST rule can know that--yesis a boolean switch or that dropping--outputdefeats a predicate onoutput. It needs the tokenizer and the checker actually executed against a real sandbox log. Prevents: The 🔴 "boolean switch before a positional silently swallows it", "_split_flagsloses the value of an unambiguous--flag=-value", "ignore_flagsdefault silently defeats an explicitflags:predicate", the malformed-line/non-list[str]-argvvacuous pass, and the uncovered missing-flag branch (cli_called.py:105). - A criteria-wide degraded-input conformance test. Iterate the
CriterionRegistryand, for each criterion, run it against four synthetic artifact states — absent, empty, syntactically malformed, and right-format-wrong-shape — asserting the result either setserroror scores 0.0, and never silently returns 1.0. This encodes the rule the PR itself states at cli_called.py:196-199 ("a missing log is a harness fault … what stops a negative guard from passing vacuously") and thejson_check.py:89precedent as a suite-wide invariant instead of a per-checker convention, with an explicit opt-out list for criteria whose absence genuinely means zero. Why not static: Requires constructing sandboxes and executing every checker to observe the returnedCriterionResult; the property is about runtime output, not code shape (CE039 catches only the specific counter-never-reaches-score pattern). Prevents: The 🔴 silent-drop finding (cli_called.py:217/126) generally, and its recurrence in the next file-reading criterion. - A watchdog-escape test: a wedged criterion must not outlive
task_timeout. Add a test that runs a task whose criterion blocks insideasyncio.to_thread(a sleeping or catastrophically-backtracking_check_impl) and assert the run terminates attask_timeoutand the process exits — todaycriteria/base.py:403-410offloads to the default executor, theThreadedWatchdog(orchestrator.py:489-493) cancels only the asyncio task, and the non-daemon worker thread is joined at interpreter exit, so the harness wedges past its own cap. Pair with a size cap on grading reads (CE036) and consider a per-criterion wall-clock budget. Why not static: The wedge is an interaction between the watchdog, asyncio cancellation and CPython's non-daemon executor-thread join at shutdown — only observable by running a real timeout to completion. Prevents: The availability half of "agent-controlled invocation log is parsed with no resource bounds" (whole-file read + per-value regex backtracking on a thread that survives the cancel). - A diff-scoped coverage gate for
src/coder_eval/criteria/**in CI. The global 80% floor let the new module ship at 95.5% branch coverage with its five negative branches (105, 116, 126, 214, 223) entirely unexecuted. Add a per-module / changed-files gate (e.g.diff-coveron the PR diff, or a--cov-fail-under=100job scoped tocoder_eval/criteria/**) so a new criterion's failure branches cannot merge unpinned — these are exactly the branches that decide pass vs. fail. Why not static: Coverage is a runtime measurement of executed arcs; no lint rule can tell whether a branch has a test. Prevents: "No test for the most common negative shape: a required flag predicate against an invocation that omits the flag entirely", and the two adjacent unpinned negatives at cli_called.py:116 and :126. - A YAML-shorthand round-trip matrix for scalar-shorthand fields. For every field whose contract advertises "a bare scalar means X", load the criterion from real YAML text with each scalar type (
model: gemini,retries: 3,ratio: 1.5,verbose: true) and assert either successful coercion or an actionable, field-anchored message. Todayflags: {retries: 3}fails with an opaqueInput should be a valid dictionary or instance of FlagMatch [type=model_type], andverbose: truemust NOT be coerced toequals: "true"(a value-less switch is recorded as""at cli_called.py:76) — so the test needs to pin the message that points the author at{equals: ""}/{absent: true}. Why not static: Needs the YAML→pydantic path executed and the resulting error text asserted; "the docstring promises unconditional coercion but the validator gates onisinstance(value, str)" is a prose-vs-code mismatch outside AST reach. Prevents: "Bare-scalarflags:shorthand coerces onlystr, so unquoted YAML numeric/boolean flag values fail with an opaque pydanticmodel_typeerror". - A new-criterion merge checklist with one real end-to-end consumer. Require a new criterion type to ship with (a) a
tasks/*.yaml(or fixture task) that actually uses it, exercised throughcoder-eval plan, and (b) registry entries in CE030'sDOCUMENTED_MODELSand CE033's doc surfaces. Keep it a checklist rather than a hard gate: the mechanical version ("every union member appears intasks/**.yaml") would need five exemptions today —file_matches_regex,reference_comparison,uipath_eval,skill_triggered,cli_calledall lack in-tree consumers — so a gate would be mostly allowlist. A single real consumer is what turns speculative API surface into extracted need. Why not static: "Is this predicate speculative?" is a judgment about need, not a code pattern; CE031-style dead-config detection can't see it because the checker does read every predicate field by name. Prevents: "FlagMatchships a 5-way predicate union with 4 validation rules for a criterion no in-tree task uses yet", and would have surfaced the ergonomics findings (logvspath, the missing presence predicate, numeric flag values) before the API froze.
Top 5 Priority Actions
- Fix the boolean-switch heuristic in src/coder_eval/criteria/cli_called.py:67-78 by making the value-vs-switch decision declarable per flag instead of guessed, because
--yes proj-1parses as{'yes': ['proj-1']}so amin_count: 0, max_count: 0guard scores 1.0 on the exact destructive invocation it forbids while the positive form of the same assertion scores 0.0. - Make an explicit
flags:predicate win overignore_flags— subtractflags.keys()from the ignore set at src/coder_eval/criteria/cli_called.py:128 or reject the overlap in_validate_boundsat src/coder_eval/models/criteria.py:441 — since the defaultignore_flags: ["output"](src/coder_eval/models/criteria.py:431) makesflags: {output: {absent: true}}score 1.0 against argv that did pass--output json, and makesflags: {output: "json"}unpassable. - Stop swallowing unusable records in src/coder_eval/criteria/cli_called.py:215-223 and :124-126: return
score=0.0with anerrornaming the log (at minimum whenevermax_countis set) and count non-list[str]argvintomalformed, because today an unparseable or differently-shaped line holding the forbidden call leaves the score at 1.0 witherror=None, contradicting the fail-loud missing-log rationale 12 lines above at :195-205. - Close the vacuous-config hole at src/coder_eval/models/criteria.py:445 with a falsiness-symmetric facet guard (
if not self.verb and not self.positional and not self.flags and not self.tool) plusmin_length=1onverb(:402) andany_of(:312), sinceverb: ""— reachable from a null${row.verb}dataset cell via task_loader.py:325 — is accepted and then matches every record for a silent 1.0. - Bind the equals form directly in a single pass at src/coder_eval/criteria/cli_called.py:52-55 rather than re-applying the space-form heuristic at :70-74, because
--offset=-1parses to{'offset': [''], '1': ['']}— anequals: "-1"predicate can never match and the fabricated flag name1/foospuriously fails unrelatedabsentpredicates — and add the dash-leading and missing-flag (cli_called.py:105) tests that are currently uncovered.
Stats: 0 🔴 · 4 🟠 · 5 🟡 · 10 🔵 across 8 axes reviewed.
|
Thanks — this was a genuinely useful review. I reproduced every finding against PR HEAD before touching anything, and all of them held. Fixed in 42e4b2a. The four 🟠 collapse into one theme worth naming: a 🟠 4 — boolean switch swallows a positionalThe root cause was guessing. Value binding is now declared: a flag consumes the following token only if it appears in I took your Implementing this surfaced something neither of us flagged: with undeclared flags treated as switches, 🟠 1 — vacuous configs
🟠 2 — silently dropped recordsUnparseable lines and non- 🟠 3 —
|
ReviewVerified locally on this branch: VerdictFundamentally correct and a good fit. It follows the extension contract exactly: pure Pydantic model in It does not overlap One real defect
verb: "ixp fields delete"
positional: ["proj-1"]
ignore_flags: ["verbose"]
min_count: 0
max_count: 0against Same failure mode as the Fix: make ignored flags value-bearing only when also listed in Smaller gaps
|
uipreliga
left a comment
uipreliga
left a comment
There was a problem hiding this comment.
Approved, but can you fix this issue:
One real defect
ignore_flags re-opens the exact hole commits 42e4b2a/fa1e328 closed.
cli_called.py:146-148 folds ignore into the value-bearing set. That's right
for --output json (the default), but wrong for any ignored switch. Confirmed
by running it:
verb: "ixp fields delete"
positional: ["proj-1"]
ignore_flags: ["verbose"]
ignore_flags re-opens the exact hole commits 42e4b2a/fa1e328 closed. cli_called.py:146-148 folds
ignore into the value-bearing set. That's right for --output json (the default), but wrong for
any ignored switch. Confirmed by running it:
verb: "ixp fields delete"
positional: ["proj-1"]
ignore_flags: ["verbose"]
min_count: 0
max_count: 0
against argv: ["ixp","fields","delete","--verbose","proj-1"] → _record_matches returns False, so
the guard PASSES on the delete it exists to forbid. --verbose swallowed proj-1. Same failure mode
as the --yes proj-1 bug, same blast radius (silent false PASS on a destructive call), reached
through the one field where value-binding is still guessed rather than declared. Fix: make
ignored flags value-bearing only when also in value_flags (and put output in a default
value_flags), or add a separate ignore_switches. Whichever you pick, it wants a test mirroring
test_boolean_switch_before_a_positional_does_not_swallow_it.
A test that shadows a CLI with a recording mock can only assert on what ran via file_matches_regex over a flattened log line. That flat string cannot express "verb X was called AND flag Y had value Z" without stacked lookaheads, cannot distinguish a quoted argument containing spaces from two arguments, and cannot stop a match running across shell operators. cli_called reads a JSON Lines invocation log — one object per invocation, `argv` required, `tool`/`exit`/`ts` optional — and matches element-wise on verb, positional arguments, and per-flag predicates (equals / contains / matches_regex / any_of / absent), with min_count/max_count bounds. Deliberate semantics: - `verb` is an ORDERED PREFIX of the non-flag arguments, so `labellings confirm` is never satisfied by `labellings unconfirm`. Assertion matching must be stricter than the permissive token-subset matching a mock dispatcher wants. - Unlisted flags are ignored, and `ignore_flags` defaults to ["output"], so grading never depends on --output json — a flag that does not change which resource an invocation addresses. - `absent: true` distinguishes "flag not passed" from "passed with a different value", which a plain dict[str, str] cannot express. - A missing log FAILS rather than counting as zero matches: otherwise `max_count: 0` would pass vacuously against a mock writing to the wrong path, which is exactly the guard such a criterion exists to provide. - Exactly one predicate per flag, so a conjunction on one flag needs either two criteria or one matches_regex spanning both. `matches_regex` therefore carries `flags` (mirroring FileMatchesRegexCriterion) — DOTALL is the usual need, since a heredoc-built payload spans lines. Setting `flags` beside a non-regex predicate is rejected rather than silently ignored. Regex survives as a per-flag predicate, scoped to one value instead of a whole line, compiled once per (pattern, flags) rather than per record, and validated up front so a bad pattern names the flag it came from. 38 new tests. Full suite, ruff, and all 166 custom lint rules pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…passing Review of this PR found that a `max_count: 0` guard could return 1.0 on exactly the invocation it forbids, by three independent routes. All three reproduced against PR HEAD; each now has a test written in the failing direction. 1. Argv tokenization guessed value binding: "the next token is the value unless it starts with -". So `fields delete --yes proj-1` bound `yes=proj-1` and emptied the positionals, and a guard on `positional: [proj-1]` PASSED while the log proved the delete happened. `--yes`/`--force`/`-y` before the target is how destructive CLIs are invoked, i.e. the shape a negative guard most needs to catch. Value binding is now DECLARED: a flag consumes the following token only if it appears in `flags:`, the new `value_flags:`, or `ignore_flags:`. Everything else is a switch and its neighbour stays positional, so ambiguity resolves toward catching the call rather than missing it. The equals form binds directly (it is unambiguous), which also fixes `--offset=-1` dropping its value and inventing a flag named `1`; a declared value flag binds a dash-leading value, so `--limit -1` works. 2. Unparseable lines and records whose `argv` is not list[str] were skipped with the score untouched, so a truncated record of the forbidden call let the guard pass. They are now a harness fault on the same footing as a missing log — score 0.0 with an error naming the count — matching this file's own documented reasoning and json_check's precedent. Also logged, since every degraded path here was previously silent. 3. `verb: ""` and `verb: " "` slipped past an `is None` facet guard and matched EVERY record, scoring 1.0; `any_of: []` satisfied the exactly-one-predicate rule and then matched nothing. Both rejected now, the facet guard is falsiness-symmetric, and a predicate on a flag also in `ignore_flags` is rejected rather than silently unevaluable. Also from the review: `re.error` is not a ValueError subclass, so the pre-flight regex guard missed a bad `flags` int and lost the flag name; widened. Dropped the `_compiled` lru_cache — `re.compile` already memoizes (re._MAXCACHE == 512), so it was redundant and retained task-supplied patterns module-wide. The unreachable `return False` in `_flag_matches` now raises, so a predicate added without a matcher arm fails loudly instead of silently scoring 0.0. Fixed the model docstring's negative example, which was unconstructible (`max_count: 0` without `min_count: 0` is rejected). 52 criterion tests (was 38). Differential against the downstream suite still agrees 7/7 on both trajectories; full suite, ruff, pyright and all 166 custom lint rules pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ken a guard
Asking why the guide's delete example does not also assert `--yes` surfaced a
defect introduced by the previous commit: declaring a flag in `flags:` promoted
it to value-bearing, so asserting a boolean switch made it swallow the following
positional. On a guard over `fields delete --yes proj-1`, adding
`flags: {yes: ""}` rebound `yes=proj-1`, emptied the positionals, and returned
1.0 — reintroducing the exact false pass that declared value-binding exists to
prevent, triggered by trying to assert MORE.
A predicate that only tests presence needs no value, so `FlagMatch.needs_value`
now excludes it from the value-bearing set, and `present: bool` joins the
predicate union as the correct way to assert a switch (`equals: ""` depends on
how the mock records one and breaks on `--force true`).
The guide now states the asymmetry this question exposed: negative guards want
the FEWEST facets that capture the forbidden act, because `max_count: 0` passes
when nothing matches, so every extra facet is another escape route — `--yes` is
not the forbidden act, and asserting it lets a `-y` spelling through. Positive
assertions want the opposite.
Closes the review's finding 10 (no way to express "flag was passed"), which was
deferred as additive; it is load-bearing after all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A predicate matches a flag NAME, so `--yes` and `-y` were unrelated flags and "confirmed with either spelling" was inexpressible. Splitting them into one criterion per spelling works for a guard (criteria are ANDed, so both are forbidden) but not for a positive assertion, which then demands BOTH. Worse, `absent` was silently wrong across spellings: an ANDed pair of absent-guards flagged EVERY invocation whatever it did, because whichever spelling was not used is always absent. That is a correctness trap, not a missing convenience. `aliases` gathers values across every name a predicate owns: `present` holds if any appeared, `absent` only if none did, and a value predicate matches if any value under any name satisfies it (so `-f X` binds like `--fields X`, with the alias joining the value-bearing set). A flag may belong to only one predicate -- an alias that is also another key, that names its own key, or that appears in ignore_flags is rejected at load time, since ambiguous ownership would make the verdict depend on dict order. Closes the last of the review's flag-matching findings. 253 criterion tests; differential against the downstream suite still 7/7 on both trajectories. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ignore_flags was folded into the value-bearing set to keep `--output json` from leaving `json` in the positionals. That made every ignored flag value-bearing, including switches: with ignore_flags: ["verbose"], `delete --verbose proj-1` bound verbose=proj-1, emptied the positionals, and a max_count: 0 guard on positional: [proj-1] returned 1.0 on the delete it forbade — the same failure as the --yes bug, through the last field where binding was still guessed. Value binding is now declarations only (flag predicates that need a value, plus value_flags, which defaults to ["output"]). An ignored flag that takes a value says so in value_flags. Also from the review: - min_count: 0 with no max_count is satisfied by every log, so the criterion could never fail; rejected alongside the other vacuities. - A missed positive reported only counts. Failure details now echo up to three recorded argvs, for a criterion whose purpose is "what did it actually run". - CLAUDE.md said 14 criterion types and omitted cli_called. - Documented that bundled short flags are not split (-rf is one flag named rf, so absent: true on f passes despite it). - Trimmed Field descriptions to behaviour; the rationale lives in comments. 68 criterion tests. Differential against the downstream suite still 7/7. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
607cd22 to
5366688
Compare
|
Fixed in The defectYou were right, and it was mine — introduced in the same commit that fixed the Reproduced before changing anything: Took your first option: binding is declarations only (predicates that need a value, plus Worth recording that this was the third source of implicit binding, all three introduced or left by me and each found only after someone hit the symptom:
Root cause on my side: after switching to declared binding I widened the declared set until the suite went green, rather than narrowing it to what correctness required — and I wrote inverse tests for every bug you named while writing none for the mechanism I was introducing. The differential kept reporting 7/7 throughout because all seven of its cases use genuine value flags ( Smaller gaps
Rebase noteRebased onto Since both approvals are on |
4 participants
Note
Merge this first. #73 stacks on top and adds
sandbox.record_cli, which generates the recording shims that produce this log — the answer to "who writes it?". #73 depends on this PR (it defaultslog:to the generated path and its round-trip test grades withcli_called), so this one lands first. Worth reviewing the pair together; ideally cut a single release after both, so downstream migrates once.Why
When a task shadows a CLI with a recording mock, the only way to assert on what actually ran is
file_matches_regexover a flattened log line. That forces patterns like this (real, from a downstream suite):The lookahead exists only because "verb X was called and flag Y had value Z" has no expression over a serialized string. Three concrete failures of the flat-string approach:
--instructions "extract total, tax"from two arguments.--corrections\s+[^|;&]*f-100to stop a match running across shell operators.What
cli_calledreads a JSON Lines invocation log the sandbox produced and matches it element-wise.Log format — only
argvis required; unknown keys are ignored so a mock may record more:{"ts": 1785416844.987, "tool": "uip", "argv": ["ixp", "projects", "get", "proj-1"], "exit": 1}Deliberately not
uip-specific: the contract is the log format, so any recording mock for any executable can emit it.toollets one log serve several shadowed executables.Semantics worth reviewing
verbis an ordered prefix of the non-flag arguments, not a token subset.ixp labellings confirmmust never be satisfied byixp labellings unconfirm. An assertion matcher has to be stricter than the permissive matching a mock dispatcher wants, where being generous is a feature.ignore_flagsdefaults to["output"].--output jsondoes not change which resource an invocation addresses, so grading must not depend on whether the agent typed it.absent: truedistinguishes "flag not passed" from "passed with a different value" — the reasonflagsis a predicate map rather thandict[str, str].max_count: 0would pass vacuously against a mock that wrote to the wrong path, which is exactly the guarantee a negative guard is supposed to provide.-. Without a CLI grammar there is no better rule; the ambiguity is confined to one place instead of duplicated into every task's pattern.--flag=valueis normalized to the space form,--terminates flag parsing, and repeated flags accumulate.Regex survives as
matches_regex, scoped to a single flag value instead of a whole line.Testing
68 unit tests in
tests/test_cli_called_criterion.py— each predicate, ordered-prefix rejection, positional ordering, repeated flags,--/lone--argv edges, count bounds, DOTALL, invalid-regex reporting, missing/empty/malformed logs, and model validation. Plus thecli_calledpayload inMINIMAL_PAYLOADS(the union parity assert requires it).Beyond unit tests, this was validated against the suite that will consume it —
UiPath/skills, whose recorder is already merged (#2375).1. Differential grading vs the criteria it replaces. Seven real criteria from that suite, replayed through its actual merged recorder (extracted from
origin/mainat merge7d4e5aa5, not a working copy), graded by both the existingfile_matches_regexand thecli_calledtranslation — on a correct trajectory and an incorrect one, since agreement on only the correct one is satisfied by a criterion that always passes:That exercise is also what produced the
flagsfield: the heredoc case needsDOTALL, and without it the translation required an[\s\S]*workaround for somethingfile_matches_regexalready supported.2. A live agent run. One IXP smoke task, real
uipath-ixpskill,tempdirdriver, graded by twocli_calledcriteria (one positive, onemax_count: 0guard):The log the agent actually produced, from the preserved sandbox:
{"ts":1785763282.590,"tool":"uip","argv":["ixp","projects","get-taxonomy","my_invoices-f1afa9ef-ixp","--output","json"],"exit":1} {"ts":1785763291.563,"tool":"uip","argv":["ixp","projects","configure-model","my_invoices-f1afa9ef-ixp","--model","gemini_2_5_pro","--output","json"],"exit":1}Real agent → real skill → real recorder → real log → this criterion. Nothing stubbed.
3. The version gate, confirmed. The same task YAML is correctly rejected by coder-eval
v0.9.0(union_tag_invalidoncli_called), so a consumer cannot adopt it before release — which is why the downstream migration waits on a pin bump.Local gates:
make lint166 passed ·ruff checkclean ·pyrightat its 3 pre-existingopenai_codexerrors · full suite 3599 passed, 5 failed, the same 5 that fail onmainatcc2cfc7(3×test_reports_stats_nonfinite, 2×test_sandboxsymlink tests needing Windows privileges). Developed on Windows/Python 3.13.9, so this repo's Linux CI is the authoritative check.Notes for a reviewer
ignore_flagsdefaults to["output"]. This is the most opinionated line in the PR: it encodes "never grade on--output json" — outcome-invisible, often the CLI default — into a criterion otherwise free of any particular CLI's conventions. I think it earns its place, but it is a judgement call and worth challenging.live_stop_polaritiesis deliberately left empty, so astop_early:block cannot arm oncli_called. It reads a sandbox file rather thanturn_records, so it genuinely is not decidable mid-run. (Verified after rebasing onto feat(early-stop)!: per-criterion arming via stop_early blocks on live criteria #78, which replaced the oldstop_whenfield with per-criterionstop_early:arming — this criterion needed no change.)matches_regex. Documented in the guide; wideningcontainstostr | list[str]later would be non-breaking if it proves annoying.🤖 Generated with Claude Code