feat(record_cli): serve a different canned response per invocation - #150
feat(record_cli): serve a different canned response per invocation#150alexandrujircan wants to merge 2 commits into
Conversation
A `record_cli` shim answered every invocation with one fixed exit_code/stdout/stderr, so an agent whose next step depends on what the tool just told it could not be evaluated: `uip ixp dummy1` and `uip ixp dummy2` got the same reply. Each entry may now declare `responses`, a list of rules tried in declaration order, first match wins, falling back to the entry's own three fields for anything no rule claims. `exit_code` defaults to 0 on a rule (the opposite of the entry default of 1): a rule exists because the author described that invocation. `when` is not a second pattern language. The criterion's matcher moved to `argv_match.py` -- stdlib-only, plain dicts -- and both surfaces lower to one spec dict, so the pattern that serves a response is the pattern that grades it. `render_recorder` embeds that module's SOURCE into the shim, which runs where coder_eval is not installed; CE047 keeps its imports stdlib-only, since one package import there would make every shadowed CLI die with an ImportError the agent reads as "the tool is broken". `FlagMatch` moved to the new cycle-free leaf `models/cli_match.py` alongside `CliMatch` and the shared verb/flag validators: models/sandbox.py cannot import from models/criteria.py, which already takes RECORD_CLI_LOG from it. Two deliberate divergences from the criterion, both tested: `ignore_flags` is empty on a rule (grading must not depend on --output; dispatch may), and `tool` stays criterion-only, addressing a log record rather than argv. Also fixes a pre-existing silent no-match: a flag written into a verb (`verb: "ixp projects get --output json"`) validated and then matched nothing, because a verb is compared against the non-flag arguments -- the criterion scored 0 against a log holding that exact call. Now rejected on every surface, reusing the splitter's own is_number rule so `head -1` stays legal. The shim records `"rule": <index>` when a rule answered, and omits the key when none did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @alexandrujircan's task in 2m 27s —— View job Code Review in Progress
|
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:150
Scope: pr:150 · branch feat/record-cli-per-invocation-responses · 2067d2b · 2026-09-02T13:02Z · workflow variant
Change class: complex — introduces a new shared argv-matching engine, a source-embedding mechanism that injects module source into generated sandbox shims, and new validation semantics that change which task YAML is accepted
This PR lands a genuinely well-engineered unification of the cli_called criterion and the record_cli response matcher — security, error handling, and architecture are clean (10/10, 10/10, 9.7/10), with new lint rules (CE047), a parity test, and an embedded-shim design that keeps the sandbox stdlib-only — but the new response-rule surface silently swallows eval-config faults: an uncompilable matches_regex loads without error and makes the shim serve the fallback response with a log line byte-identical to a legitimate no-match (src/coder_eval/invocation_log.py:110), so a task can score differently for identical agent behaviour with no signal on any report surface; that one defect chain, plus an untyped model/matcher seam that fails open, is the real risk, and everything else is dead code, doc drift, and test-coverage gaps — bottom line: high-quality change, merge after closing the silent-misgrade path.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 9.4 / 10 | 0 | 0 | 1 | 1 | FlagMatch.needs_value is dead after the matcher extraction while argv_match.predicate_needs_value still documents a parity contract nothing enforces |
| 2. Type Safety | 8.5 / 10 | 0 | 1 | 1 | 0 | FlagMatch.matches_regex is never compiled at validation time, so an invalid pattern on a record_cli response rule loads cleanly and silently serves the wrong canned response |
| 3. Test Health | 9 / 10 | 0 | 0 | 2 | 0 | Guards on the duplicated CliMatch/CliCalledCriterion facet surface are incomplete: parity check is one-directional and the rule-side defaults are pinned by no test |
| 4. Security | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 5. Architecture & Design | 9.7 / 10 | 0 | 0 | 0 | 3 | MergeField(strategy="replace") on RecordedCli.responses is unreachable metadata the resolver never reads, yet a new test row pins it as if enforced |
| 6. Error Handling & Resilience | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 7. API Surface & Maintainability | 9.3 / 10 | 0 | 0 | 1 | 2 | A responses rule shadowed by a preceding rule is accepted silently, with no load-time error and no runtime diagnostic |
| 8. Evaluation Harness Quality | 9 / 10 | 0 | 1 | 0 | 0 | Shim's except Exception around select_rule silently serves the fallback response, leaves no trace in calls.jsonl, and skips every later rule (also untested) |
Overall Score: 9.4 / 10 · Weakest Axis: Type Safety at 8.5 / 10
Totals: 🔴 0 · 🟠 2 · 🟡 5 · 🔵 6 across 8 axes.
Blockers
-
[Axis 2] FlagMatch.matches_regex is never compiled at validation time, so an invalid pattern on a record_cli response rule loads cleanly and silently serves the wrong canned response (
src/coder_eval/models/cli_match.py:125) —FlagMatch._exactly_one_predicatevalidates the combination of predicates but never validates the regex itself — the only reference tomatches_regexin the validator is the no-op guard at line 125:if self.flags and self.matches_regex is None:
msg = f"FlagMatch.flags applies only to matches_regex, but the predicate is {set_predicates[0]!r}"
On main this was tolerable because the single consumer, criteria/cli_called.py:59-70, does a pre-flight re.compile(predicate.matches_regex, predicate.flags) and returns error=f"Invalid matches_regex for flag '{name}': {exc}". This PR exposes the same unvalidated model through a SECOND surface — CliMatch.flags -> dict[str, FlagMatch] (cli_match.py:314) reached via CliResponse.when (models/sandbox.py:346) — which has no equivalent guard anywhere. grep -n 're\.compile' src/coder_eval/models/cli_match.py returns nothing, and re is not even imported there.
Reproduced on the PR HEAD worktree:
CliResponse(when={'verb':'get','flags':{'val':{'matches_regex':'([unclosed'}}}, stdout='RULE-ANSWER')
-> validated OK
Rendering that entry's shim and executing it with get --val x gives:
rc 1 stdout 'DEFAULT' stderr "coder_eval recorder: response matching failed: PatternError('unterminated character set at position 1')"
log: {"ts": ..., "tool": "uip", "argv": ["get", "--val", "x"], "exit": 1}
The except Exception in the generated respond() (invocation_log.py:110-115) swallows the PatternError and returns the entry defaults, and record() omits the "rule" key — so the log line is byte-identical to a legitimate "no rule matched". The agent is told the tool failed, its next step diverges, the task scores wrong, and the only trace is a stderr line that goes to the agent rather than to the run report.
Fix: compile in the model, where BOTH surfaces get it. Add to _exactly_one_predicate (after the line-125 guard):
if self.matches_regex is not None:
try:
re.compile(self.matches_regex, self.flags)
except (re.error, ValueError) as exc:
raise ValueError(f"FlagMatch.matches_regex is not a valid regex with flags={self.flags}: {exc}") from exc
That also covers the bad-flags-int case tests/test_cli_called_criterion.py:645 currently only catches at check time (flags: 99999999), and lets criteria/cli_called.py:59-70 be deleted as redundant. Add a test asserting CliResponse(when={'flags': {'val': {'matches_regex': '([unclosed'}}}) raises ValidationError — tests/test_sandbox_record_cli.py has no such case today (grep -n matches_regex tests/test_sandbox_record_cli.py returns nothing).
2. [Axis 8] Shim's except Exception around select_rule silently serves the fallback response, leaves no trace in calls.jsonl, and skips every later rule (also untested) (src/coder_eval/invocation_log.py:110) — respond() in the shim template reads:
try:
selected = select_rule(RULES, list(argv))
except Exception as exc:
sys.stderr.write("coder_eval recorder: response matching failed: %r\\n" % (exc,))
return EXIT_CODE, STDOUT_TEXT, STDERR_TEXT, None
(invocation_log.py:108-115). The None in the 4th slot means record() omits the "rule" key, so an eval-config fault is byte-identical in the log to a legitimate no-match, and ONE faulting rule aborts select_rule for ALL rules of that tool.
This is reachable today: neither FlagMatch nor CliMatch validates that matches_regex compiles (src/coder_eval/models/cli_match.py:47 declares it as a plain str | None; _exactly_one_predicate at :109-128 does not compile it). Verified by rendering and executing a real shim from RecordedCli(tool='uip', exit_code=1, stderr='uip: unknown command\n', responses=[CliResponse(when={'verb':'ixp x','flags':{'model':{'matches_regex':'([unclosed'}}}, stdout='MATCHED\n')]):
returncode: 1
stdout: ''
stderr: "coder_eval recorder: response matching failed: PatternError('unterminated character set at position 1')\nuip: unknown command\n"
log: {"ts": ..., "tool": "uip", "argv": ["ixp", "x", "--model", "pro"], "exit": 1}
The agent receives the failure response instead of MATCHED, its next step diverges, and the task scores differently — for identical agent behaviour — with no signal any report surfaces. Note the asymmetry: the cli_called half of the SAME pattern already pre-validates and reports loudly (src/coder_eval/criteria/cli_called.py:57-70: re.compile(predicate.matches_regex, predicate.flags) → error=f"Invalid matches_regex for flag '{name}': {exc}"), so the two surfaces this PR set out to unify diverge exactly where it matters.
Fix: move that pre-flight into FlagMatch._exactly_one_predicate (src/coder_eval/models/cli_match.py:109) so an uncompilable pattern is a load-time error on BOTH surfaces and the shim's except Exception becomes genuinely unreachable; and when it does fire, record a distinguishable key (e.g. "rule_error": "<repr>") so cli_called can fail the log the way it already fails on .error sentinels and unusable records. Also add a test that renders a shim with a faulting rule and asserts the log record — the shim body lives inside _TEMPLATE (a string literal), so ruff, pyright and even CE005 (no-silent-except) see nothing there; that whole branch class is currently unguarded by any static check.
Non-blocking, but please consider before merge
- [Axis 1]
FlagMatch.needs_valueis dead after the matcher extraction whileargv_match.predicate_needs_valuestill documents a parity contract nothing enforces (src/coder_eval/models/cli_match.py:86) —grep -rn "needs_value" .over the whole repo (excluding .git/node_modules) returns exactly four hits, none of which reads the property:src/coder_eval/models/cli_match.py:87(the definition), andsrc/coder_eval/argv_match.py:121/:127/:175(the replacement). Before this PR the property was live —criteria/cli_called.py::_record_matchescalledp.needs_valuewhen buildingvalue_flags. The PR moved that call toargv_match.py:175(if predicate_needs_value(predicate)) and left the 13-line property behind:
@property
def needs_value(self) -> bool: # cli_match.py:86-98 — no caller anywhere
...
return not (self.present or self.absent)What makes this more than an unused member is argv_match.py:127, which asserts the opposite: "Mirrors FlagMatch.needs_value; both sides of the spec boundary must agree or a rule and the criterion grading it would parse the same argv differently." A maintainer changing the presence-predicate rule reads that sentence, edits FlagMatch.needs_value, sees no behavior change, and has to discover by bisection that only predicate_needs_value is wired. CE037 ("no unreferenced module-level private helper in src/") does not fire here because this is a public property on a model class, not a module-level private function.
Fix: delete FlagMatch.needs_value (cli_match.py:86-98) and reword argv_match.py:127 to name the single implementation, or — if the property is wanted as the pydantic-side spelling — make build_match_spec use it so the "mirror" claim is true. Consider widening CE037 to public properties/methods on src/coder_eval/models/ classes that no code reads by name, which would have caught this mechanically.
2. [Axis 2] The lowered match spec crosses the model/matcher boundary as an untyped dict[str, Any] whose every key is read with a permissive .get(...) or <default>, so a producer/consumer key mismatch widens the match instead of failing (src/coder_eval/argv_match.py:160) — build_match_spec (models/cli_match.py:244-264) declares -> dict[str, Any] and is surfaced as public API on two models — CliMatch.match_spec (cli_match.py:346) and CliCalledCriterion.match_spec (models/criteria.py:571) — then consumed by a criterion at criteria/cli_called.py:30 (argv_matches(criterion.match_spec, argv)) and by the embedded shim. Every guarantee the pydantic models establish is erased at that seam and re-derived by key lookup with a permissive fallback:
argv_match.py:162 flag_specs: dict[str, Any] = spec.get("flags") or {}
argv_match.py:171 ignore = frozenset(spec.get("ignore_flags") or ())
argv_match.py:177 | frozenset(spec.get("value_flags") or ())
argv_match.py:185 spellings = spec.get("verb_spellings") or []
argv_match.py:197 expected = spec.get("positional")
Every required key is optional to the reader, and the fallback for a missing key is always "unconstrained" — the failure direction that makes a rule match everything or a criterion score 1.0 on any log. pyright cannot see this (Any), and the tests only partly can: I renamed "value_flags" to "valueflags" in build_match_spec and 3 tests failed (good), but renaming "ignore_flags" to "ignored_flags" left all 188 tests in test_cli_called_criterion.py + test_cli_match_parity.py + test_sandbox_record_cli.py passing.
The stdlib-only constraint (CE047) does NOT justify Any here: typing is already on CE047's allowlist (tests/lint/rules/ce047_embedded_shim_stdlib_only.py:36, STDLIB_ALLOWED = frozenset({"re", "json", "os", "sys", "time", "shlex", "itertools", "typing"})) and argv_match.py already does from typing import Any at line 29. Declare the contract instead of documenting it in the module docstring (argv_match.py:16-25):
class FlagPredicate(TypedDict):
equals: str | None; contains: str | None; matches_regex: str | None
any_of: list[str] | None; absent: bool; present: bool
aliases: list[str]; flags: int
class MatchSpec(TypedDict):
verb_spellings: list[list[str]]; positional: list[str] | None
flags: dict[str, FlagPredicate] | None; value_flags: list[str]; ignore_flags: list[str]
Then annotate build_match_spec(...) -> MatchSpec, both match_spec properties, and argv_matches(spec: MatchSpec, ...), and index required keys directly (spec["verb_spellings"], spec["ignore_flags"]) rather than .get(...) or .... TypedDict is closed, so a key renamed on either side becomes a pyright error at both. Adjacent nit on the same seam: criteria/cli_called.py:21 widens to record: dict[str, Any] although its producer invocation_log.parse_log already returns the narrower dict[str, object] (invocation_log.py:199) and the only read is record.get("tool") — dict[str, object] works there unchanged.
3. [Axis 3] Guards on the duplicated CliMatch/CliCalledCriterion facet surface are incomplete: parity check is one-directional and the rule-side defaults are pinned by no test (tests/test_cli_match_parity.py:23) — test_both_surfaces_declare_every_match_facet only loops the hard-coded tuple — for field in MATCH_FACET_FIELDS: assert field in CliMatch.model_fields ... assert field in CliCalledCriterion.model_fields — and test_criterion_adds_only_non_argv_fields only subtracts over set(CliCalledCriterion.model_fields). Proven by mutation: adding env: dict[str, str] | None = Field(default=None, description="NEW FACET added only to CliMatch") to CliMatch leaves uv run --extra dev pytest tests/test_cli_match_parity.py at 20 passed. Add the closing assertion assert set(CliMatch.model_fields) == set(MATCH_FACET_FIELDS) so a facet added to the rule surface must be added to MATCH_FACET_FIELDS, which then forces it onto cli_called via the existing loop.
4. [Axis 3] Rendered-shim invariant tests only ever render the rules-less shim; the pure-ASCII invariant is silently false for the rules-bearing (embedded-source) shape (tests/test_sandbox_record_cli.py:493) — All three invariants render the shim shape that does NOT embed the matcher: line 495 source = render_recorder(RecordedCli(tool="uip")) (pure-ASCII), line 485 (no coder_eval import), line 479 (no exec). The embedded half is the only half that can violate any of them, and it already violates one: render_recorder(RecordedCli(tool='uip', responses=[CliResponse(when={'verb':'ixp dummy1'})])).encode('ascii') raises UnicodeEncodeError: 'ascii' codec can't encode character '—' in position 876, from the em-dash on src/coder_eval/argv_match.py:1 ("""Structured argv matching — the one engine both CLI surfaces share.). Parametrize the three tests over both a rules-less and a rules-bearing RecordedCli, then either drop the ASCII invariant deliberately (the shim is written encoding="utf-8" at src/coder_eval/sandbox.py:630, and Python 3 source defaults to UTF-8) or make argv_match.py ASCII-clean.
5. [Axis 7] A responses rule shadowed by a preceding rule is accepted silently, with no load-time error and no runtime diagnostic (src/coder_eval/models/sandbox.py:412) — responses: list[CliResponse] = MergeField( (src/coder_eval/models/sandbox.py:412) has no model_validator over the list, so nothing rejects a rule that a preceding rule already claims. Verified by executing at PR HEAD: RecordedCli(tool='uip', responses=[{'when': {'verb': 'ixp x'}, 'stdout': 'a'}, {'when': {'verb': 'ixp x'}, 'stdout': 'b'}]) -> 'exact duplicate accepted: 2', and the prefix case [{'when': {'verb': 'ixp projects'}}, {'when': {'verb': 'ixp projects get'}}] -> 'shadowed rules accepted: 2'. This is inconsistent with the rest of the same authoring surface, which hard-errors on every other declaration that can never take effect: validate_verbs at src/coder_eval/models/cli_match.py:191-197 rejects one verb_any_of entry prefixing another with 'the shorter one already accepts every invocation the longer one does'; validate_flag_ownership at :233-241 rejects a predicate on an ignored flag; validate_positional at :200-207 rejects positional: []; and Sandbox._setup_record_cli (src/coder_eval/sandbox.py:623-627) raises RuntimeError when two entries would write the same shim filename. Minimum viable fix: add a model_validator(mode='after') on RecordedCli that rejects two rules whose when.match_spec compare equal (cheap and exact), and extend it to the verb-prefix case that validate_verbs already knows how to detect. The docs currently rely on prose alone (docs/TASK_DEFINITION_GUIDE.md:598: 'put the specific rule above the general one').
Nits
- [Axis 1] Stray unbalanced parenthesis introduced in the CE-rules paragraph of CLAUDE.md (
CLAUDE.md:223) — The CE047 insertion doubled a closing paren on the preceding CE039 clause: the line now reads... and \# noqa: CE039` the cases that really are the agent's)), CE047 (a module whose SOURCE is embedded .... The pre-PR text endedthe agent's).with a single)`. Drop one paren so the parenthetical closes once. - [Axis 5]
MergeField(strategy="replace")onRecordedCli.responsesis unreachable metadata the resolver never reads, yet a new test row pins it as if enforced (src/coder_eval/models/sandbox.py:412) — models/sandbox.py:412 declaresresponses: list[CliResponse] = MergeField(withstrategy="replace", and tests/test_merge_strategy_annotations.py:37 asserts(RecordedCli, "responses", "replace"). Butmerge_strategy_ofis read at only two sites (orchestration/config_merge.py:226 inside_merge_dict_by_model, and :337 inresolve_root's top-level loop), both of which iteratemodel_types— models reached by deep dict merge.SandboxConfig.record_cliis itselfMergeField(strategy="replace")on alist, and_merge_valuereturnsnew_valoutright for"replace"(config_merge.py:209return new_val # replace), soRecordedCliis never a merge root and the annotation is never consulted. CE014's_MERGE_ROOT_CLASSES(tests/lint/rules/ce014_merge_strategy_declared.py:45-63) correctly omitsRecordedCli, so nothing required it. Failure scenario: a future author reads the annotation plus the field description ("Replaced (not merged) across config layers") and the pinning test, concludesresponsesparticipates in layer merging, and changes it tostrategy="append"expecting an experiment variant to add a rule to a task's list — nothing happens, because the enclosingrecord_clilist already replaced wholesale. Either drop theMergeField(use plainField) and the test row, or addRecordedClito CE014's scope only if the engine is taught to descend into list elements. - [Axis 5] CE047 guards the embedded module's imports but nothing guards the textual splice's shared namespace, and a collision would fail silently into "every invocation gets the fallback response" (
src/coder_eval/invocation_log.py:50) — invocation_log.py:50 splices the whole ofargv_match.pyinto the shim's module namespace ({matcher_source}, filled at :194 by_MATCHER_SECTION.format(source=_matcher_source()) if rules else ""), sitting betweenRULES = {rules!r}(:49) and the shim's owndef record(:56) /def respond(:99) /def main(:122). CE047 checks imports only (STDLIB_ALLOWED, ce047:36); nothing checks top-level NAMES. Failure scenario:argv_match.pygains a module-level helper namedrecord(a plausible name for an argv matcher that logs, and there is no rule against it). The shim's laterdef record(argv, exit_code, rule)rebinds it, soselect_rule-> the matcher's internal call torecord(...)raisesTypeError;respondswallows it at :110-115 (except Exception ... return EXIT_CODE, STDOUT_TEXT, STDERR_TEXT, None) and EVERY invocation silently falls back to the entry defaults, with no"rule"key in the log to distinguish it from "no rule matched". Cheapest fix: reserve the shim's own globals in CE047 — flag any module-leveldef/assignment in an embedded module whose name is in{TOOL, EXIT_CODE, STDOUT_TEXT, STDERR_TEXT, RULES, SHIM_DIR, LOG_PATH, LOG_ERROR_PATH, record, respond, main}— or prefix the shim's own names (_ce_record,_ce_respond,_ce_main) so the two namespaces cannot collide by construction. - [Axis 5] "Which module is embedded into a shim" has two hardcoded sources of truth (invocation_log filename vs CE047 path regex) with no cross-check, so the rule can go vacuous (
tests/lint/rules/ce047_embedded_shim_stdlib_only.py:32) — The embedded-module identity is written twice: invocation_log.py:163return resources.files("coder_eval").joinpath("argv_match.py").read_text(encoding="utf-8")and ce047:32_EMBEDDED = re.compile(r"[/\\\\]coder_eval[/\\\\]argv_match\\.py$"). CE047's own tests (tests/test_custom_lint.py:159) synthesize the path string too —path = "src/coder_eval/argv_match.py" if embedded else ...— so no test ever asserts the regex matches a file that actually exists in the tree. Failure scenario: a later refactor moves the matcher tocoder_eval/shim/argv_match.pyand updates_matcher_source()accordingly; CE047's regex still requirescoder_eval/argv_match.py, matches nothing, and passes vacuously. A subsequentfrom coder_eval.models import FlagMatchadded to the moved module then ships, and every shadowed CLI dies withImportErrorinside the sandbox — the exact defect CE047 exists to prevent, surfacing to the agent as "the tool is broken". Fix: derive CE047's target set from one constant (e.g.EMBEDDED_MODULES = ("argv_match.py",)exported byinvocation_logand imported by the rule), and add a repo-level assertion that at least one realsrc/file matches — a lint rule that guards zero files must fail, not pass. - [Axis 7] The guide's own
when:example is a hard ValidationError when pasted into thecli_calledcriterion it claims parity with (docs/TASK_DEFINITION_GUIDE.md:597) — docs/TASK_DEFINITION_GUIDE.md:597 states 'whentakes the same facets ascli_called... evaluated by the same matcher, so the pattern that serves a response is the pattern that grades it', and the example directly above it (lines 587-591) usesverb: "ixp projects get"/positional: ["proj-1"]/flags: {output: json}. Verified at PR HEAD: that block validates as a rule (CliMatch.model_validate({...})-> ok,ignore_flags=[]) but the identical facets on the criterion raise —CliCalledCriterion(description='d', verb='ixp projects get', positional=['proj-1'], flags={'output':'json'})-> "cli_called flag predicate(s) 'output' are also listed in ignore_flags (directly or as an alias), which drops them before matching" (src/coder_eval/models/cli_match.py:236-241), because the criterion'signore_flagsdefaults to['output'](src/coder_eval/models/criteria.py:552-559). The bullet at line 600 explains why the defaults differ but not that this makes the documented example non-transferable. Fix: use a non-outputflag in the example (e.g.flags: {model: gemini_2_5_pro}, the flag the shared parity cases at tests/test_cli_match_parity.py:52-55 deliberately chose for exactly this reason), or qualify the line-597 claim with 'except that a rule may key on a flag the criterion ignores by default'. - [Axis 7] Task YAML that previously loaded now fails validation, with no BREAKING CHANGE footer so the generated CHANGELOG will not say so (
src/coder_eval/models/cli_match.py:177) — src/coder_eval/models/cli_match.py:175-183 addsif token.startswith("-") and token != "-" and not is_number(token.lstrip("-")): raise ValueError(...), which makesverb: "ixp projects get --output json"a hard validation error oncli_called— YAML that loaded onorigin/main(it validated, then silently matched nothing). The error message itself is excellent and names the fix ('Put it inflags:instead, e.g. flags: {output: }'), and docs/TASK_DEFINITION_GUIDE.md:1008 calls the rejection out. The gap is release signalling: the single commit isfeat(record_cli): serve a different canned response per invocationwith no!and noBREAKING CHANGE:footer, and pyproject.toml:403 setscommit_parser = "conventional"withmajor_on_zero = false(:406), so python-semantic-release will emit only a 'Features' entry — an adopter upgrading a minor version gets a task-load failure with nothing in CHANGELOG.md pointing at it. Fix: add aBREAKING CHANGE: a flag written insidecli_called/whenverb:is now rejected at load; move it toflags:.footer to the commit so the generated CHANGELOG carries a BREAKING CHANGES section (the version bump stays minor undermajor_on_zero = false, which is fine).
What's Missing
Parallel paths:
- 🟡 The rendered shim now carries the grading pattern in clear text inside the sandbox:
RULES = [{'when': {'verb_spellings': [['ixp','projects','configure-model']], 'flags': {'model': {'equals': 'pro', ...}}}}]sits in a world-readable 755 file on the agent's PATH, socat "$(command -v uip)"hands the agent the exact invocation acli_calledcriterion grades (the PR's own premise is that the pattern that serves is the pattern that grades). The repo's established anti-cheat path was not extended in parallel — nostage_reference_dir-style shielding, notasks/anti_cheat_*probe next totasks/anti_cheat_reference/, and nodocs/TASK_DEFINITION_GUIDE.mdnote that aresponses:block is agent-visible. Before this PR the shim leaked only exit code / stdout text; state the exposure explicitly (a shim must stay readable to be executable) and add a probe task, or task authors will keep assuming the pattern is private. (trigger: src/coder_eval/invocation_log.py) - 🟠 The regex pre-flight landed on only one of the two surfaces the PR set out to unify:
criteria/cli_called.py:57-70compilesmatches_regexup front and reports a named error, while the new response-rule path (CliResponse.when->CliMatch.flags) compiles lazily inside the shim and swallows the failure. The parallel guard belongs inFlagMatch._exactly_one_predicate, where both surfaces inherit it. (trigger: src/coder_eval/models/cli_match.py) (restates: Axis 2: FlagMatch.matches_regex is never compiled at validation time) - 🔵 CE047 was added for the imports half of the splice contract only; the other halves of "this file's whole source is pasted into a generated module" got no guard — top-level names colliding with the shim's own
record/respond/main/RULES, and module-level executable statements or anif __name__block that would run inside the shim. Extend CE047 (reserved-name + no-top-level-side-effect checks) rather than leaving the import check as the only enforced clause. (trigger: tests/lint/rules/ce047_embedded_shim_stdlib_only.py) (restates: Axis 5: CE047 guards the embedded module's imports but nothing guards the textual splice's shared namespace)
Tests:
- 🟡 Predicate coverage stops at
equals/present/absent:SHARED_CASEShas no row forcontains,matches_regex,any_of,aliases, or the regexflagsint, andgrep -n 'any_of\|contains\|matches_regex\|aliases' tests/test_sandbox_record_cli.pyreturns nothing — so no executed shim ever exercises them. Those are exactly the predicates where the two surfaces can diverge (the criterion pre-compiles regexes, the shim compiles lazily and swallows the error), so the parity suite proves parity only for the predicates that cannot drift. Add one shared case per FlagMatch predicate plus one executed-shim test with amatches_regex/any_of/verb_any_ofrule. (trigger: tests/test_cli_match_parity.py) - 🟡 All four
TestRenderedSourceinvariants (valid-Python, no-exec, no-coder_eval-import, pure-ASCII) render onlyRecordedCli(tool="uip")— the shape that does NOT embed the matcher and therefore cannot violate any of them; the embedded shape already breaks the ASCII one. Parametrize the class over a rules-less and a rules-bearing spec. (trigger: tests/test_sandbox_record_cli.py) (restates: Axis 3: Rendered-shim invariant tests only ever render the rules-less shim) - 🟡 The facet-parity guard runs in one direction only (
MATCH_FACET_FIELDS-> both models, and criterion -> non-facet subtraction); a facet added toCliMatchalone is invisible, verified by mutation (addingenvtoCliMatchleaves the file at 20 passed). Close it withassert set(CliMatch.model_fields) == set(MATCH_FACET_FIELDS). (trigger: tests/test_cli_match_parity.py) (restates: Axis 3: Guards on the duplicated CliMatch/CliCalledCriterion facet surface are incomplete) - 🔵 No load-time test asserts that an uncompilable
matches_regex(or a bogusflagsint) is rejected on either surface —grep -n matches_regex tests/test_sandbox_record_cli.pyreturns nothing, and the criterion's only coverage (tests/test_cli_called_criterion.py:184,:645) asserts a check-time score of 0.0, not aValidationError. Add theValidationErrorcase alongside whichever validator fix lands. (trigger: tests/test_sandbox_record_cli.py) (restates: Axis 2: FlagMatch.matches_regex is never compiled at validation time) - 🔵 The new user-authored
responses:/when:config block is documented in the guide but pinned by no doc-parity guard: CE030'sDOCUMENTED_MODELS(tests/lint/doc_schema_parity.py:44-48) tracks onlyTaskDefinition/RunLimits/Dataset/SimulationConfig, and nested models are excluded by design, soRecordedCli/CliResponse/CliMatchfields can be added or renamed with the guide silently going stale. Either addRecordedCli+CliResponseto the tracked list, or record the exemption. (trigger: docs/TASK_DEFINITION_GUIDE.md) - 🔵
render_recordernow depends on a runtime package-source read (resources.files("coder_eval").joinpath("argv_match.py").read_text(...), invocation_log.py:163), but every test resolves it from the source tree; nothing exercises the installed-distribution path the docker image and the published wheel actually use. A build that ever ships bytecode-only or excludes the module fails at sandbox setup for every rules-bearing entry — add a packaging assertion (or a test that imports from an installed wheel) so the dependency on shipped.pysource is explicit. (trigger: src/coder_eval/invocation_log.py) - 🔵
responses:has no in-repo task exercising it end-to-end:grep -rn record_cli tasks/returns nothing, although the neighbouring feature shipstasks/mock_path_dirs_smoke.yaml. The whole feature is proven only by unit tests that callrender_recorder/_run_shimdirectly, so no run ever proves a real agent gets rule-dependent output through PATH resolution under the driver a user runs. (trigger: src/coder_eval/models/sandbox.py)
Downstream consumers:
- 🟡 The new
"rule": <index>log key has no reader anywhere:parse_logpasses the record through,_record_matches(criteria/cli_called.py:21-30) reads onlytool,cli_calledgained norulefacet, and no report or evalboard surface renders it. So the one signal that distinguishes "rule 2 answered" from "nothing matched and you got the entry default" is write-only — which is also why a mis-ordered or shadowed rule set stays invisible to grading. Either givecli_calleda way to assert on it, or surface it in the run report / task.json. (trigger: src/coder_eval/invocation_log.py) - 🟡 The breaking rejection of a flag inside
verb:is documented on the repo-only surface (docs/TASK_DEFINITION_GUIDE.md:1008) but not on the plugin surface installed users author against:plugins/coder-eval/reference/criteria.md:89still describesverbwith no mention of the restriction, and CE033 stays green because the PR changed the validator, not the fielddescription=strings the reference is generated from.record_cli/responseshas no plugin-side reference at all, so/coder-eval:taskand/coder-eval:lint-taskscannot teach or lint either. Reword theverbfield descriptions (which regenerates the reference) and consider a sandbox section in the plugin reference. (trigger: src/coder_eval/models/cli_match.py) - 🔵 The guide actively encourages copying a pattern between the two surfaces ("the pattern that serves a response is the pattern that grades it"), but its own
when:example (flags: {output: json}) is a hardValidationErrorwhen pasted ontocli_called, because the criterion'signore_flagsdefaults to["output"]— the deliberate divergence is explained one bullet later without saying it makes the example non-transferable. (trigger: docs/TASK_DEFINITION_GUIDE.md) (restates: Axis 7: The guide's ownwhen:example is a hard ValidationError when pasted into thecli_calledcriterion)
Daily/nightly:
- 🟡 The PR states no blast radius for the drive-by breaking change, and this repo cannot show it:
grep -rln cli_called tasks/returns nothing, so every task that could be rejected by the new verb-flag validator lives in the downstreamcoder-eval-uipatheval-runner suite theuip ixp …examples come from. A task that fails schema validation is reported as a SKIPPED task, so the failure mode on a nightly run is quiet coverage loss rather than a red run, and the commit carries noBREAKING CHANGE:footer for the generated CHANGELOG. State the downstream migration ("move the flag intoflags:") and how the nightly surfaces it before this ships. (trigger: src/coder_eval/models/cli_match.py) (restates: Axis 7: Task YAML that previously loaded now fails validation, with no BREAKING CHANGE footer)
Harness & Lint Improvements
Static checks (lint / type):
- [ce-lint] CE048 — lint the RENDERED shim, not just the module it embeds. New
tests/lint/rules/entry wired as a dedicated lint test class intests/test_custom_lint.py(the CE033/CE035 whole-tree pattern, not aBaseRuleintests/lint/runner.py): for every shapeinvocation_log.render_recordercan emit (at minimum rules-less and rules-bearing), render totmp/, then (a) runtests/lint/runner.check_file(all CE rules) and (b) runruff check --isolated --select F,E9over the output; any violation failsmake lint. Today the whole shim body lives inside the_TEMPLATEstring literal atsrc/coder_eval/invocation_log.py:30-135, so ruff, pyright and CE005 see literally nothing there — CE047 exists only because the author already hit this blind spot for the embedded module's imports, and its own docstring (tests/lint/rules/ce047_embedded_shim_stdlib_only.py:13-15) says so. Verified on the PR HEAD worktree: the rendered rules-bearing shim is clean under both gates today (check_file->[], ruff F/E9 -> pass), and injecting a module-leveldef record(a)into the embedded matcher source makes ruff emitF811 Redefinition of unused 'record' from line 257— i.e. the check fires exactly on the collision scenario. Note CE005 would NOT flag today'sexcept Exception(it writes to stderr, so_body_handles_erroris satisfied); the value here is that every FUTURE bareexcept: pass, undefined name (F821), or shadowed global inside the template becomes a hard gate instead of a silent sandbox-only failure. Prevents: A8-high / A6 / A3 (theexcept Exceptionatinvocation_log.py:110and its whole branch class being invisible to every static gate) and A5-low (shim-globals namespace collision with the splicedargv_match.py— reproduced as ruff F811). - [ce-lint] CE049 — a user-authored regex field must compile at load time. Add
RegexPattern = Annotated[str, AfterValidator(_must_compile)]tosrc/coder_eval/models/and a rule that flags anystr/str | Nonefield on aBaseModelundersrc/coder_eval/models/whose name matches(^|_)(regex|pattern)s?$unless it uses that alias (or the module compiles it in a validator). Detection is a pure AnnAssign+name check. It would have fired onFlagMatch.matches_regex(models/cli_match.py:47), whose only pre-flight guard lives in ONE of its two consumers (criteria/cli_called.py:57-70), so the newrecord_cliresponse-rule surface reached throughCliResponse.whengets none. The same rule finds four pre-existing instances of the identical class —models/criteria.py:425(file_matches_regex.pattern),:624,:892(command_pattern),:921(exclude_pattern) andmodels/mutations.py:42— so adopting it is a one-shot migration to the shared alias, after which an uncompilable pattern fails atcoder-eval planinstead of mid-run. Prevents: A2-high (invalidmatches_regexon arecord_cliresponse rule loads cleanly and silently serves the wrong canned response) and the root cause of A8-high (the shim'sexcept Exceptiononly becomes reachable because nothing validates the pattern at load). - [ce-lint] CE014 extension — derive the merge-reachable class set from the engine, and forbid
MergeFieldon a model the engine can never reach.tests/lint/rules/ce014_merge_strategy_declared.py:45-63hardcodes_MERGE_ROOT_CLASSES. Replace it with a set computed from the three-Droots by walkingmodel_fieldsand following exactly the edgesconfig_merge._merge_valuefollows (strategy == "deep"on a nestedBaseModel/free-formdict; areplacelist is a leaf), then add the converse assertion: aMergeField(...)on a class NOT in that derived set is dead metadata and fails lint. Verified: the derived set flagsRecordedCli.responses(models/sandbox.py:412— unreachable, because the enclosingSandboxConfig.record_cliat:505is areplacelist) and simultaneously closes a real pre-existing hole in the opposite direction —DockerBuildConfig(models/sandbox.py:104) IS reachable viaDockerDriverConfig.build(:188, plainField, deep by default) but is absent from_MERGE_ROOT_CLASSES, so its list fieldssecrets(:137) andextra_args(:147) are annotated by luck, and a new list field there would be unguarded. Prevents: A5-low / A7-low (MergeField(strategy="replace")onRecordedCli.responsesis unreachable metadata that a pinning row intests/test_merge_strategy_annotations.py:37presents as enforced), plus the latentDockerBuildConfiggap the same derivation exposes. - [ce-lint] CE037 extension — dead PUBLIC member on a model class. Widen
tests/lint/rules/ce037_no_dead_private_helper.pybeyond module-level_privatedefs to cover@property/@cached_propertyand plain methods onBaseModelsubclasses undersrc/coder_eval/models/, using the same whole-treesrc/corpus grep and the same# noqa: CE037escape hatch. One implementation detail matters: CE037 currently skips any decorated def (node.decorator_listatce037:90) on the theory that a decorator is a registration —property/cached_propertyare NOT registrations and must be exempted from that exemption, or the rule stays blind exactly where this bug lives. It would have fired onFlagMatch.needs_value(models/cli_match.py:86-98), whose sole caller moved toargv_match.py:175in this PR;grep -rn needs_valueover the repo returns four hits and none reads the property. It is a plain@property, not acomputed_field, so there is no serialization reader either. Prevents: A1-medium (FlagMatch.needs_valueis dead whileargv_match.py:127documents a parity contract nothing enforces). - [pyright] Type the lowered match spec instead of passing
dict[str, Any]across the model/matcher seam. DeclareFlagPredicate/MatchSpecTypedDicts inargv_match.py(typingis already on CE047'sSTDLIB_ALLOWEDatce047:36andargv_match.py:29already imports from it, so the stdlib-only embedding constraint is no obstacle), annotatebuild_match_spec(models/cli_match.py:244), bothmatch_specproperties (cli_match.py:346,models/criteria.py:571) andargv_matches(argv_match.py:160) with it, and index required keys directly (spec["ignore_flags"]) instead ofspec.get(...) or <unconstrained-default>atargv_match.py:162/171/177/185/197. TypedDict is closed, so a key renamed on either side is a pyright error on both. Pair it with a narrow companion CE check — no-> dict[str, Any]on a public method of a model undersrc/coder_eval/models/— so the next lowering helper cannot reintroduce the seam. Also narrowcriteria/cli_called.py:21fromrecord: dict[str, Any]todict[str, object], matching what its producerinvocation_log.parse_log(invocation_log.py:199) already returns. Implementation caveat:build_match_specbuildsflagsfrommodel_dump()(typeddict[str, Any]), andselect_rulereadsrule.get("when") or {}— both need one localizedcast, which is still a large improvement over five silent fallbacks. Prevents: A2-medium (producer/consumer key mismatch widens the match instead of failing — verified by mutation: renaming the emitted keyignore_flags->ignored_flagsleaves all 188 tests in the three cli test files green and silently stops dropping--output). - [ce-lint] CE050 — one source of truth for "what gets embedded", plus a non-vacuity assertion for every path-scoped rule, plus ASCII-only embedded source. Three small changes to
tests/lint/rules/ce047_embedded_shim_stdlib_only.pyand the lint self-test: (1) exportEMBEDDED_MODULES = ("argv_match.py",)fromcoder_eval.invocation_logand have both_matcher_source()(invocation_log.py:163) and CE047's_EMBEDDEDregex (ce047:32) derive from it — today the identity is written twice andtests/test_custom_lint.py:159synthesizes the path string, so no test ever asserts the regex matches a file that exists; (2) add a generic assertion to the lint suite that every path-scoped rule's target pattern matches at least one real file in the tree — a rule guarding zero files must FAIL, not pass vacuously; (3) assert the embedded module's source is pure ASCII, or delete the ASCII invariant deliberately. Todaysrc/coder_eval/argv_match.py:1carries the file's single non-ASCII char (an em dash), which makesrender_recorder(...).encode('ascii')raise at position 853 for every rules-bearing shim whiletests/test_sandbox_record_cli.py:495asserts the property holds — because it only ever renders the rules-LESS shape. Prevents: A5-low (CE047's embedded-module identity can go vacuous after a refactor, re-admitting the exact ImportError the rule exists to stop) and the ASCII half of A3-medium. - [ce-lint] CE051 — a documented YAML example must validate against the model it claims to illustrate. Extend the existing doc-surface lint family (CE026–CE031/CE033–CE035 style: a dedicated
@pytest.mark.lintclass reasoning over Markdown): a fenced ```yaml block indocs/TASK_DEFINITION_GUIDE.mdcarrying a directive comment (e.g. ``) must `model_validate` against every model named. `docs/TASK_DEFINITION_GUIDE.md:587-597` claims '`when` takes the same facets as `cli_called` … the pattern that serves a response is the pattern that grades it', but the block above it (`flags: {output: json}`) validates as a `CliMatch` and raises `ValidationError` as a `CliCalledCriterion`, because the criterion's `ignore_flags` defaults to `["output"]` (`models/criteria.py:552-559`) and `validate_flag_ownership` (`models/cli_match.py:233-241`) rejects the overlap. A directive on that one block turns a doc claim into a gate. Prevents: A7-low (the guide's own `when:` example is a hard ValidationError when pasted into the `cli_called` criterion it claims parity with). - [ce-lint] CE052 — balanced delimiters in CLAUDE.md prose. A cheap text rule over
CLAUDE.md(and optionallydocs/*.md): outside fenced code blocks and inline-code spans, parentheses/brackets must balance per paragraph.CLAUDE.mdis a heavily append-edited file of deeply nested parentheticals, and this PR's CE047 insertion doubled a closing paren on the preceding CE039 clause (… the agent's)), **CE047** (…at CLAUDE.md:223). No human reviewer reliably catches that in a 400-word parenthetical; a 20-line rule does it every time. Scope it to balance-only (no style opinions) to keep the false-positive rate near zero. Prevents: A1-low (stray unbalanced parenthesis in the CE-rules paragraph of CLAUDE.md).
Harness improvements (not statically reachable):
- Make an eval-config fault distinguishable in the artifact the harness grades. In the shim template (
src/coder_eval/invocation_log.py:108-115), whenselect_ruleraises, record a distinguishable key ("rule_error": "<repr>") in the JSONL entry — mirroring the convention the file already has one function up, where a failed log write drops acalls.jsonl.errorsentinel precisely so 'the record was dropped' is not confusable with 'the agent never ran it'. Then makecli_calledfail loudly on that key the way it already fails on the sentinel, and add a test that renders a shim with a faulting rule, EXECUTES it, and asserts the log record (there is no such test today:grep matches_regex tests/test_sandbox_record_cli.pyreturns nothing). Also consider per-rule guarding inargv_match.select_rule(argv_match.py:223-226) so one faulting rule does not abort evaluation of every later rule for that tool. Why not static: The defect is that two JSONL records are byte-identical for two different causes — a property of the emitted artifact, not of the source. Confirming it requires rendering the shim, executing it as a subprocess, and readingcalls.jsonl; no AST rule can see that areturn ..., Nonemakes a fault indistinguishable from a legitimate no-match downstream. Prevents: A8-high / A6-high / A3-medium (theexcept Exceptionatinvocation_log.py:110serves the fallback response, leaves no trace incalls.jsonl, and skips every later rule — the task scores differently for identical agent behavior with no signal in any report). - Renderer-shape coverage: parametrize every artifact-invariant test over every shape the renderer can emit. All four
TestRenderedSourcetests (tests/test_sandbox_record_cli.py:466/479/485/495) buildRecordedCli(tool="uip")with noresponses, i.e. only the shape that does NOT embedargv_match.py— the half that cannot violate any of the three invariants they assert. Parametrize them over(rules-less, rules-bearing), which immediately turns the ASCII assertion red (UnicodeEncodeErrorat position 853, from the em dash onargv_match.py:1) and forces a deliberate decision: drop the ASCII invariant (the shim is writtenencoding="utf-8"atsandbox.py:630and Python 3 source defaults to UTF-8, so nothing actually breaks) or make the embedded module ASCII-clean. Adopt it as a standing convention for generated artifacts: a test asserting a property of rendered output must cover every branch of the renderer. Why not static: Which output shapes a renderer can emit is a semantic property of its branches (if rules:inrender_recorder), and 'this test fixture exercises only one of them' is a property of the test's arguments — neither is expressible as a lint pattern. CE050 covers only the ASCII sub-case; the general shape-coverage gap needs the fixture change. Prevents: A3-medium (rendered-shim invariant tests only ever render the rules-less shim; the pure-ASCII invariant is silently false for the embedded shape). - Close the twin-declaration parity guard by construction, not by a hardcoded list.
MATCH_FACET_FIELDSis a hand-written tuple andtests/test_cli_match_parity.py:23/28only check membership in one direction — proven by mutation: addingenv: dict[str, str] | NonetoCliMatchleaves the file at exactly 20 passed. Either derive the tuple from the rule surface (MATCH_FACET_FIELDS = tuple(CliMatch.model_fields)), which makes the drift structurally impossible, or add the closing assertionassert set(CliMatch.model_fields) == set(MATCH_FACET_FIELDS)(it evaluates True at PR HEAD, so it lands green). Generalize the habit: whenever a module comment claims 'a facet added to one surface and forgotten on the other is caught by the parity test' (models/cli_match.py:131-135), the test must assert set EQUALITY on both sides, never membership in one. Why not static: The check itself is mechanical, but the gap is in a test's assertion strength, and 'this loop covers only one direction' is not a detectable source pattern — a lint rule would have to know which two models are meant to be twins. Encoding that knowledge is exactly what the derived-tuple fix does, in the test. Prevents: A3-medium / A5-medium (facet added toCliMatchand forgotten oncli_called, or vice versa, ships silently). - Reject an unreachable
responsesrule at load, and add an in-repo net for task YAML. Add amodel_validator(mode="after")onRecordedCli(models/sandbox.py:412) that rejects two rules whosewhen.match_speccompare equal — exactly decidable, since the lowered spec is the complete input to the pureargv_matches. Extend it to the verb-prefix case ONLY when the earlier rule is verb-only (positional is None and flags is None); an unconditional prefix check is unsound, verified:[{verb: 'ixp projects', flags: {force: {present: true}}}, {verb: 'ixp projects get'}]still dispatches to rule 1. Back it with a CE034-style scan over in-repotasks/**.yaml. This aligns the surface with the rest of its own authoring contract, which already hard-errors on every other never-firing declaration (validate_verbsatcli_match.py:191-197,validate_flag_ownershipat:233-241,validate_positionalat:200-207, duplicate shim filenames atsandbox.py:624-629); today the guidance is prose only (docs/TASK_DEFINITION_GUIDE.md:598). Why not static: The offending artifact is user-authored task YAML that lives outside this repository, so no rule oversrc/ortasks/can be the primary gate — the mechanical check has to run at model-validation time. Deciding shadowing also requires comparing two LOWERED match specs (a semantic operation on validated data), not matching a source pattern. Prevents: A7-medium / A8-low (aresponsesrule shadowed by a preceding rule is accepted silently, with no load-time error and no runtime diagnostic). - A task-schema back-compat corpus plus a release-signal gate. Keep a fixtures directory of task YAML that MUST keep loading; a change that makes previously-valid YAML invalid then forces the author to delete or edit a fixture, and a CI check makes that deletion require a
BREAKING CHANGE:footer (or an explicit label). This PR addedmodels/cli_match.py:175-183, which turnsverb: "ixp projects get --output json"into a hardcli_calledload error — YAML that validated onorigin/main— while the single commit isfeat(record_cli): …with no!and no footer. Withcommit_parser = "conventional"andmajor_on_zero = false(pyproject.toml:403/406), python-semantic-release emits only a Features entry, so an adopter taking a minor bump gets a task-load failure with nothing in CHANGELOG.md pointing at it. (The rejection itself is good and its error message names the fix; only the release signalling is missing.) Why not static: Detecting 'input that used to validate no longer does' requires running the OLD and NEW validators over a corpus — a cross-version behavioral diff, not a property of one tree — and the remedy lives in commit metadata (the footer), which no AST rule can see. Prevents: A7-low (task YAML that previously loaded now fails validation, with no BREAKING CHANGE footer, so the generated CHANGELOG will not say so). - Note on CE numbering: the proposals above use CE048–CE052.
CE040,CE041andCE042return zero hits acrosssrc/,tests/,docs/andCLAUDE.mdat PR HEAD — either retired or claimed on an in-flight branch. The runner asserts id uniqueness at import time (tests/lint/runner.py:87-90) and its comment says the loser of a collision renumbers, so take the next free numbers ABOVE the highest in use (047) rather than backfilling the gap, and confirm against open branches before wiring. Why not static: This is a process note about number allocation across concurrent branches; the uniqueness invariant inside a single tree is already enforced at import time, but nothing can see another branch's claim. Prevents: A duplicate CE id (the failure the runner's own anti-shadow assertion documents) when several of these rules land in parallel.
Top 5 Priority Actions
- Compile
FlagMatch.matches_regexinside_exactly_one_predicate(src/coder_eval/models/cli_match.py:125) so an uncompilable pattern is a load-timeValidationErroron BOTH surfaces instead of only the criterion's pre-flight at criteria/cli_called.py:57-70 — today it reaches the shim and silently changes a task's score. - Make the shim's matcher-fault path observable: the bare
except Exceptionat src/coder_eval/invocation_log.py:110 must record a distinguishable key (e.g."rule_error") in calls.jsonl the wayrecord()already writes itscalls.jsonl.errorsentinel, and should guard per-rule so one faulting rule cannot abortselect_rulefor every later rule of that tool. - Add a
model_validator(mode="after")onRecordedCli.responses(src/coder_eval/models/sandbox.py:412) rejecting two rules whose loweredmatch_speccompare equal — and, gated on the earlier rule being verb-only, the verb-prefix shadowing case — matching howvalidate_verbs/validate_flag_ownershipalready hard-error on every other never-firing declaration. - Replace the untyped
dict[str, Any]match spec withTypedDicts (MatchSpec/FlagPredicate) acrossbuild_match_spec(src/coder_eval/models/cli_match.py:244) andargv_matches(src/coder_eval/argv_match.py:160) and index required keys directly, sincetypingis already on CE047's allowlist and a rename of"ignore_flags"today passes all 188 tests while silently widening matching. - Close the two guard gaps: assert
set(CliMatch.model_fields) == set(MATCH_FACET_FIELDS)in tests/test_cli_match_parity.py:23 so the parity check runs both directions, and parametrize the three rendered-shim invariants (tests/test_sandbox_record_cli.py:493) over a rules-bearingRecordedCli— the embedded shape already violates the pure-ASCII claim via the em-dash on src/coder_eval/argv_match.py:1.
Stats: 0 🔴 · 2 🟠 · 5 🟡 · 6 🔵 across 8 axes reviewed.
…a shim fault Addresses review on #150. Blocker: `FlagMatch.matches_regex` was never compiled at validation. The criterion's checker pre-flighted it, but the response-rule surface that now shares the model evaluates the pattern INSIDE the sandbox, where the shim swallowed the PatternError and served its fallback -- a log line byte-identical to a legitimate no-match. The task scored differently for identical agent behaviour, with nothing on any report surface. The compile moved into `FlagMatch`, so both surfaces refuse the pattern at load, and the now-unreachable checker pre-flight is gone. Second half of the same chain: when the shim's rule evaluation does raise, it returns the error and `record()` books it as `rule_error`, so an eval-config fault can no longer read as a clean no-match; `cli_called` fails the whole log on it, the way it already fails on the write-failure sentinel. Tested by corrupting a rendered shim -- the only route left now that the pattern cannot load. Also from the review: - The lowered spec crossed the model/matcher seam as `dict[str, Any]` read with permissive `.get(...) or <default>`, whose failure direction is always "unconstrained" -- a rule that matches everything, or a criterion that scores 1.0 on any log. It is now `MatchSpec` / `FlagPredicate` / `ResponseRule` TypedDicts with required keys indexed directly, so a key renamed on either side is a pyright error on both. Tests pin that the TypedDict key sets equal the model field sets, which is what makes the one cast honest. - `FlagMatch.needs_value` was dead after the matcher extraction while `argv_match.predicate_needs_value` documented a mirror contract nothing enforced. Deleted; the survivor now says it is the only implementation. - A `responses` rule an earlier rule already claims was accepted silently, unlike every other unusable declaration on this surface. Now a load error for the two decidable cases: an exact duplicate, and a verb-only rule whose verb prefixes a later one under the same flag parsing. - CE047 grew a namespace half: an embedded module may not bind a top-level name the shim binds itself, since the shim's definition wins and the resulting TypeError is swallowed into "every invocation gets the fallback". Its target set now derives from `invocation_log.EMBEDDED_MODULES` instead of a second hardcoded path, and a test asserts it matches a file that exists -- a rule guarding zero files must fail, not pass. - The three rendered-shim invariants only ever rendered the rules-less shape. Parametrized over both; the spliced shape did violate the ASCII one, so `argv_match.py` is ASCII-only now, by rule rather than by luck. - Parity test closed the other direction (a facet added to `CliMatch` alone passed before), `MergeField` dropped from `RecordedCli.responses` (never a merge root, so the strategy was inert metadata that read as a knob), doubled paren in CLAUDE.md, and the guide's example no longer uses the one flag a rule may key on but the criterion rejects. BREAKING CHANGE: a flag written inside a `verb:` (e.g. `verb: "ixp projects get --output json"`) is now rejected when a task loads, on `cli_called` and on a `record_cli` response rule. It previously validated and then matched nothing, so the criterion scored 0 against a log holding that exact call. Move the flag to `flags:`. An invalid `matches_regex` is likewise a load error rather than a check-time one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — that review found a real defect chain, not a hypothetical one. All two blockers and all five non-blocking items are addressed in dbc5afb. Blockers1 + 2 (one chain). That makes the checker's pre-flight unreachable, so it is deleted, and its two tests moved from check-time to load-time (including the For the second half — when the shim's rule evaluation does raise — I did not make Non-blocking
NitsAll taken: doubled paren fixed; BREAKING CHANGE footer added to dbc5afb, covering both the flag-in-verb rejection and the Verification: |
|
|
||
| from pydantic import BaseModel, ConfigDict, Field, model_validator | ||
|
|
||
| from coder_eval.argv_match import FlagPredicate, MatchSpec, is_number |

Problem
A
record_clishim answered every invocation with one fixedexit_code/stdout/stderr. So an agent whose next step depends on what the tool just told it could not be evaluated:uip ixp dummy1anduip ixp dummy2got the same reply, and anything needing a real answer fell back to a hand-written mock undermock_path_dirs.What you can write now
Rules are tried in declaration order, first match wins, and anything unclaimed gets the entry's own three fields.
exit_codedefaults to 0 on a rule — the opposite of the entry default of 1 — because a rule exists precisely because the author described that invocation. The log now carries"rule": <index>when a rule answered and omits the key when none did, so "returned the default" and "rule 2 answered, and looks like the default" are no longer the same line.whenis not a second pattern languageThe criterion's matcher moved to
src/coder_eval/argv_match.py— stdlib-only, plain dicts — and both surfaces lower to one spec dict:cli_calledcallsargv_matches(criterion.match_spec, argv); the checker lost ~160 lines.render_recorderembeds that module's source into the shim (read as a package resource), so the pattern that serves a response is the pattern that grades it. A test asserts the embedded copy is the shipped source verbatim; another asserts it is embedded only when the entry declares rules, so a shim with no rules is byte-identical to before.CE047 (new lint rule) keeps that module's imports stdlib-only: the shim runs where
coder_evaland its dependencies are not installed, and one package import there would make every shadowed CLI die with anImportErrorthe agent reads as "the tool is broken".FlagMatchmoved to the new cycle-free leafmodels/cli_match.py, alongsideCliMatchand the shared verb/flag validators —models/sandbox.pycannot import frommodels/criteria.py, which already takesRECORD_CLI_LOGfrom it.Two deliberate divergences from the criterion, both pinned by tests in
tests/test_cli_match_parity.py:cli_calledignore_flagsdefault["output"]— grading must not depend on a flag that changes nothing[]— dispatch may legitimately answer differently for--output jsontoolDrive-by fix: a flag inside a verb matched nothing, silently
verb: "ixp projects get --output json"validated and then could never match, because a verb is compared against the non-flag arguments. Silent in the worst direction:cli_calledscored 0 against a log holding that exact call. Pre-existing on the criterion; now a validation error on every surface, naming the fix. It reuses the splitter's ownis_numberrule, sohead -1stays legal.Review notes
whenis mapping-only — a barewhen: "ixp dummy1"is rejected with the{verb: ...}spelling in the message. An earlier draft accepted the string as shorthand; it was dropped so a pattern has one shape.FlagMatchkeeps its scalar shorthand (flags: {output: json}=={equals: json}). A single-valued predicate has only one facet a scalar could mean, and removing it would be a breaking change to every existing task. Happy to revisit separately if we want strict one-way-only.Verification
ruff format,ruff check,pyright, and the lint-marked suite are clean on the touched files. 4660 tests pass. Eight failures on my Windows box are pre-existing and unrelated — verified identical atorigin/mainwith these changes stashed: symlink-privilege (4),float.numeratorintest_reports_stats_nonfinite(3), and a UTF-8 decode in the CE033 drift test (1).🤖 Generated with Claude Code