Skip to content

feat(record_cli): serve a different canned response per invocation - #150

Open
alexandrujircan wants to merge 2 commits into
mainfrom
feat/record-cli-per-invocation-responses
Open

feat(record_cli): serve a different canned response per invocation#150
alexandrujircan wants to merge 2 commits into
mainfrom
feat/record-cli-per-invocation-responses

Conversation

@alexandrujircan

Copy link
Copy Markdown
Contributor

Problem

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, and anything needing a real answer fell back to a hand-written mock under mock_path_dirs.

What you can write now

sandbox:
  record_cli:
    - tool: uip
      exit_code: 1                    # fallback: anything no rule claims
      stderr: "uip: unknown command\n"
      responses:
        - when: {verb: "ixp dummy1"}
          stdout: "response1\n"
        - when: {verb: "ixp dummy2"}
          stdout: "response2\n"
        - when:                       # any cli_called facet, ANDed
            verb: "ixp projects get"
            positional: ["proj-1"]
            flags: {output: json}
          stdout: '{"id": "proj-1", "name": "Invoices"}'
        - when: {verb: "ixp projects get missing"}
          exit_code: 4
          stderr: "project not found\n"

Rules are tried in declaration order, first match wins, and anything unclaimed gets the entry's own three fields. exit_code defaults 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.

when is not a second pattern language

The criterion's matcher moved to src/coder_eval/argv_match.py — stdlib-only, plain dicts — and both surfaces lower to one spec dict:

  • cli_called calls argv_matches(criterion.match_spec, argv); the checker lost ~160 lines.
  • render_recorder embeds 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_eval and its dependencies are not installed, and 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 pinned by tests in tests/test_cli_match_parity.py:

cli_called response rule
ignore_flags default ["output"] — grading must not depend on a flag that changes nothing [] — dispatch may legitimately answer differently for --output json
tool a match facet (addresses a log record) n/a — the shim knows which tool it is

Drive-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_called scored 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 own is_number rule, so head -1 stays legal.

Review notes

  • when is mapping-only — a bare when: "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.
  • FlagMatch keeps 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.
  • Responses are stateless: a rule answers the same way however many times it matches. A per-call sequence needs a counter file that two concurrent agent commands would race on — that stays a hand-written mock.

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 at origin/main with these changes stashed: symlink-privilege (4), float.numerator in test_reports_stats_nonfinite (3), and a UTF-8 decode in the CE033 drift test (1).

🤖 Generated with Claude Code

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>
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

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


Code Review in Progress

  • Read review guidelines (.github/code_review.md)
  • Analyze the full diff
  • Review architectural changes
  • Check argv_match.py module and pattern matching
  • Review new lint rule CE047
  • Verify test coverage and parity tests
  • Check for security and performance implications
  • Provide structured feedback

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review: coder_eval — pr: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

  1. [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_predicate validates the combination of predicates but never validates the regex itself — the only reference to matches_regex in 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 ValidationErrortests/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

  1. [Axis 1] FlagMatch.needs_value is dead after the matcher extraction while argv_match.predicate_needs_value still 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), and src/coder_eval/argv_match.py:121 / :127 / :175 (the replacement). Before this PR the property was live — criteria/cli_called.py::_record_matches called p.needs_value when building value_flags. The PR moved that call to argv_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

  1. [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 ended the agent's).with a single)`. Drop one paren so the parenthetical closes once.
  2. [Axis 5] MergeField(strategy="replace") on RecordedCli.responses is 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 declares responses: list[CliResponse] = MergeField( with strategy="replace", and tests/test_merge_strategy_annotations.py:37 asserts (RecordedCli, "responses", "replace"). But merge_strategy_of is read at only two sites (orchestration/config_merge.py:226 inside _merge_dict_by_model, and :337 in resolve_root's top-level loop), both of which iterate model_types — models reached by deep dict merge. SandboxConfig.record_cli is itself MergeField(strategy="replace") on a list, and _merge_value returns new_val outright for "replace" (config_merge.py:209 return new_val # replace), so RecordedCli is 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 omits RecordedCli, 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, concludes responses participates in layer merging, and changes it to strategy="append" expecting an experiment variant to add a rule to a task's list — nothing happens, because the enclosing record_cli list already replaced wholesale. Either drop the MergeField (use plain Field) and the test row, or add RecordedCli to CE014's scope only if the engine is taught to descend into list elements.
  3. [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 of argv_match.py into the shim's module namespace ({matcher_source}, filled at :194 by _MATCHER_SECTION.format(source=_matcher_source()) if rules else ""), sitting between RULES = {rules!r} (:49) and the shim's own def 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.py gains a module-level helper named record (a plausible name for an argv matcher that logs, and there is no rule against it). The shim's later def record(argv, exit_code, rule) rebinds it, so select_rule -> the matcher's internal call to record(...) raises TypeError; respond swallows 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-level def/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.
  4. [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:163 return 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 to coder_eval/shim/argv_match.py and updates _matcher_source() accordingly; CE047's regex still requires coder_eval/argv_match.py, matches nothing, and passes vacuously. A subsequent from coder_eval.models import FlagMatch added to the moved module then ships, and every shadowed CLI dies with ImportError inside 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 by invocation_log and imported by the rule), and add a repo-level assertion that at least one real src/ file matches — a lint rule that guards zero files must fail, not pass.
  5. [Axis 7] The guide's own when: example is a hard ValidationError when pasted into the cli_called criterion it claims parity with (docs/TASK_DEFINITION_GUIDE.md:597) — docs/TASK_DEFINITION_GUIDE.md:597 states 'when takes the same facets as cli_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) uses verb: "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's ignore_flags defaults 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-output flag 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'.
  6. [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 adds if token.startswith("-") and token != "-" and not is_number(token.lstrip("-")): raise ValueError(...), which makes verb: "ixp projects get --output json" a hard validation error on cli_called — YAML that loaded on origin/main (it validated, then silently matched nothing). The error message itself is excellent and names the fix ('Put it in flags: instead, e.g. flags: {output: }'), and docs/TASK_DEFINITION_GUIDE.md:1008 calls the rejection out. The gap is release signalling: the single commit is feat(record_cli): serve a different canned response per invocation with no ! and no BREAKING CHANGE: footer, and pyproject.toml:403 sets commit_parser = "conventional" with major_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 a BREAKING CHANGE: a flag written inside cli_called/when verb: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 under major_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, so cat "$(command -v uip)" hands the agent the exact invocation a cli_called criterion 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 — no stage_reference_dir-style shielding, no tasks/anti_cheat_* probe next to tasks/anti_cheat_reference/, and no docs/TASK_DEFINITION_GUIDE.md note that a responses: 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-70 compiles matches_regex up 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 in FlagMatch._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 an if __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_CASES has no row for contains, matches_regex, any_of, aliases, or the regex flags int, and grep -n 'any_of\|contains\|matches_regex\|aliases' tests/test_sandbox_record_cli.py returns 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 a matches_regex / any_of / verb_any_of rule. (trigger: tests/test_cli_match_parity.py)
  • 🟡 All four TestRenderedSource invariants (valid-Python, no-exec, no-coder_eval-import, pure-ASCII) render only RecordedCli(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 to CliMatch alone is invisible, verified by mutation (adding env to CliMatch leaves the file at 20 passed). Close it with assert 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 bogus flags int) is rejected on either surface — grep -n matches_regex tests/test_sandbox_record_cli.py returns nothing, and the criterion's only coverage (tests/test_cli_called_criterion.py:184, :645) asserts a check-time score of 0.0, not a ValidationError. Add the ValidationError case 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's DOCUMENTED_MODELS (tests/lint/doc_schema_parity.py:44-48) tracks only TaskDefinition / RunLimits / Dataset / SimulationConfig, and nested models are excluded by design, so RecordedCli / CliResponse / CliMatch fields can be added or renamed with the guide silently going stale. Either add RecordedCli + CliResponse to the tracked list, or record the exemption. (trigger: docs/TASK_DEFINITION_GUIDE.md)
  • 🔵 render_recorder now 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 .py source 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 ships tasks/mock_path_dirs_smoke.yaml. The whole feature is proven only by unit tests that call render_recorder / _run_shim directly, 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_log passes the record through, _record_matches (criteria/cli_called.py:21-30) reads only tool, cli_called gained no rule facet, 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 give cli_called a 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:89 still describes verb with no mention of the restriction, and CE033 stays green because the PR changed the validator, not the field description= strings the reference is generated from. record_cli / responses has no plugin-side reference at all, so /coder-eval:task and /coder-eval:lint-tasks cannot teach or lint either. Reword the verb field 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 hard ValidationError when pasted onto cli_called, because the criterion's ignore_flags defaults 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 own when: example is a hard ValidationError when pasted into the cli_called criterion)

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 downstream coder-eval-uipath eval-runner suite the uip 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 no BREAKING CHANGE: footer for the generated CHANGELOG. State the downstream migration ("move the flag into flags:") 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 in tests/test_custom_lint.py (the CE033/CE035 whole-tree pattern, not a BaseRule in tests/lint/runner.py): for every shape invocation_log.render_recorder can emit (at minimum rules-less and rules-bearing), render to tmp/, then (a) run tests/lint/runner.check_file (all CE rules) and (b) run ruff check --isolated --select F,E9 over the output; any violation fails make lint. Today the whole shim body lives inside the _TEMPLATE string literal at src/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-level def record(a) into the embedded matcher source makes ruff emit F811 Redefinition of unused 'record' from line 257 — i.e. the check fires exactly on the collision scenario. Note CE005 would NOT flag today's except Exception (it writes to stderr, so _body_handles_error is satisfied); the value here is that every FUTURE bare except: 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 (the except Exception at invocation_log.py:110 and its whole branch class being invisible to every static gate) and A5-low (shim-globals namespace collision with the spliced argv_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)] to src/coder_eval/models/ and a rule that flags any str / str | None field on a BaseModel under src/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 on FlagMatch.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 new record_cli response-rule surface reached through CliResponse.when gets 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) and models/mutations.py:42 — so adopting it is a one-shot migration to the shared alias, after which an uncompilable pattern fails at coder-eval plan instead of mid-run. Prevents: A2-high (invalid matches_regex on a record_cli response rule loads cleanly and silently serves the wrong canned response) and the root cause of A8-high (the shim's except Exception only becomes reachable because nothing validates the pattern at load).
  • [ce-lint] CE014 extension — derive the merge-reachable class set from the engine, and forbid MergeField on a model the engine can never reach. tests/lint/rules/ce014_merge_strategy_declared.py:45-63 hardcodes _MERGE_ROOT_CLASSES. Replace it with a set computed from the three -D roots by walking model_fields and following exactly the edges config_merge._merge_value follows (strategy == "deep" on a nested BaseModel/free-form dict; a replace list is a leaf), then add the converse assertion: a MergeField(...) on a class NOT in that derived set is dead metadata and fails lint. Verified: the derived set flags RecordedCli.responses (models/sandbox.py:412 — unreachable, because the enclosing SandboxConfig.record_cli at :505 is a replace list) and simultaneously closes a real pre-existing hole in the opposite direction — DockerBuildConfig (models/sandbox.py:104) IS reachable via DockerDriverConfig.build (:188, plain Field, deep by default) but is absent from _MERGE_ROOT_CLASSES, so its list fields secrets (:137) and extra_args (:147) are annotated by luck, and a new list field there would be unguarded. Prevents: A5-low / A7-low (MergeField(strategy="replace") on RecordedCli.responses is unreachable metadata that a pinning row in tests/test_merge_strategy_annotations.py:37 presents as enforced), plus the latent DockerBuildConfig gap the same derivation exposes.
  • [ce-lint] CE037 extension — dead PUBLIC member on a model class. Widen tests/lint/rules/ce037_no_dead_private_helper.py beyond module-level _private defs to cover @property / @cached_property and plain methods on BaseModel subclasses under src/coder_eval/models/, using the same whole-tree src/ corpus grep and the same # noqa: CE037 escape hatch. One implementation detail matters: CE037 currently skips any decorated def (node.decorator_list at ce037:90) on the theory that a decorator is a registration — property / cached_property are NOT registrations and must be exempted from that exemption, or the rule stays blind exactly where this bug lives. It would have fired on FlagMatch.needs_value (models/cli_match.py:86-98), whose sole caller moved to argv_match.py:175 in this PR; grep -rn needs_value over the repo returns four hits and none reads the property. It is a plain @property, not a computed_field, so there is no serialization reader either. Prevents: A1-medium (FlagMatch.needs_value is dead while argv_match.py:127 documents a parity contract nothing enforces).
  • [pyright] Type the lowered match spec instead of passing dict[str, Any] across the model/matcher seam. Declare FlagPredicate / MatchSpec TypedDicts in argv_match.py (typing is already on CE047's STDLIB_ALLOWED at ce047:36 and argv_match.py:29 already imports from it, so the stdlib-only embedding constraint is no obstacle), annotate build_match_spec (models/cli_match.py:244), both match_spec properties (cli_match.py:346, models/criteria.py:571) and argv_matches (argv_match.py:160) with it, and index required keys directly (spec["ignore_flags"]) instead of spec.get(...) or <unconstrained-default> at argv_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 under src/coder_eval/models/ — so the next lowering helper cannot reintroduce the seam. Also narrow criteria/cli_called.py:21 from record: dict[str, Any] to dict[str, object], matching what its producer invocation_log.parse_log (invocation_log.py:199) already returns. Implementation caveat: build_match_spec builds flags from model_dump() (typed dict[str, Any]), and select_rule reads rule.get("when") or {} — both need one localized cast, 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 key ignore_flags -> ignored_flags leaves 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.py and the lint self-test: (1) export EMBEDDED_MODULES = ("argv_match.py",) from coder_eval.invocation_log and have both _matcher_source() (invocation_log.py:163) and CE047's _EMBEDDED regex (ce047:32) derive from it — today the identity is written twice and tests/test_custom_lint.py:159 synthesizes 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. Today src/coder_eval/argv_match.py:1 carries the file's single non-ASCII char (an em dash), which makes render_recorder(...).encode('ascii') raise at position 853 for every rules-bearing shim while tests/test_sandbox_record_cli.py:495 asserts 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.lint class reasoning over Markdown): a fenced ```yaml block in docs/TASK_DEFINITION_GUIDE.md carrying 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 optionally docs/*.md): outside fenced code blocks and inline-code spans, parentheses/brackets must balance per paragraph. CLAUDE.md is 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), when select_rule raises, 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 a calls.jsonl.error sentinel precisely so 'the record was dropped' is not confusable with 'the agent never ran it'. Then make cli_called fail 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.py returns nothing). Also consider per-rule guarding in argv_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 reading calls.jsonl; no AST rule can see that a return ..., None makes a fault indistinguishable from a legitimate no-match downstream. Prevents: A8-high / A6-high / A3-medium (the except Exception at invocation_log.py:110 serves the fallback response, leaves no trace in calls.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 TestRenderedSource tests (tests/test_sandbox_record_cli.py:466/479/485/495) build RecordedCli(tool="uip") with no responses, i.e. only the shape that does NOT embed argv_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 (UnicodeEncodeError at position 853, from the em dash on argv_match.py:1) and forces a deliberate decision: drop the ASCII invariant (the shim is written encoding="utf-8" at sandbox.py:630 and 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: in render_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_FIELDS is a hand-written tuple and tests/test_cli_match_parity.py:23/28 only check membership in one direction — proven by mutation: adding env: dict[str, str] | None to CliMatch leaves 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 assertion assert 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 to CliMatch and forgotten on cli_called, or vice versa, ships silently).
  • Reject an unreachable responses rule at load, and add an in-repo net for task YAML. Add a model_validator(mode="after") on RecordedCli (models/sandbox.py:412) that rejects two rules whose when.match_spec compare equal — exactly decidable, since the lowered spec is the complete input to the pure argv_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-repo tasks/**.yaml. This aligns the surface with the rest of its own authoring contract, which already hard-errors on every other never-firing declaration (validate_verbs at cli_match.py:191-197, validate_flag_ownership at :233-241, validate_positional at :200-207, duplicate shim filenames at sandbox.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 over src/ or tasks/ 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 (a responses rule 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 added models/cli_match.py:175-183, which turns verb: "ixp projects get --output json" into a hard cli_called load error — YAML that validated on origin/main — while the single commit is feat(record_cli): … with no ! and no footer. With commit_parser = "conventional" and major_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, CE041 and CE042 return zero hits across src/, tests/, docs/ and CLAUDE.md at 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

  1. Compile FlagMatch.matches_regex inside _exactly_one_predicate (src/coder_eval/models/cli_match.py:125) so an uncompilable pattern is a load-time ValidationError on 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.
  2. Make the shim's matcher-fault path observable: the bare except Exception at src/coder_eval/invocation_log.py:110 must record a distinguishable key (e.g. "rule_error") in calls.jsonl the way record() already writes its calls.jsonl.error sentinel, and should guard per-rule so one faulting rule cannot abort select_rule for every later rule of that tool.
  3. Add a model_validator(mode="after") on RecordedCli.responses (src/coder_eval/models/sandbox.py:412) rejecting two rules whose lowered match_spec compare equal — and, gated on the earlier rule being verb-only, the verb-prefix shadowing case — matching how validate_verbs/validate_flag_ownership already hard-error on every other never-firing declaration.
  4. Replace the untyped dict[str, Any] match spec with TypedDicts (MatchSpec/FlagPredicate) across build_match_spec (src/coder_eval/models/cli_match.py:244) and argv_matches (src/coder_eval/argv_match.py:160) and index required keys directly, since typing is already on CE047's allowlist and a rename of "ignore_flags" today passes all 188 tests while silently widening matching.
  5. 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-bearing RecordedCli — 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>
@alexandrujircan

Copy link
Copy Markdown
Contributor Author

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.

Blockers

1 + 2 (one chain). FlagMatch.matches_regex now compiles in the model validator, so both surfaces refuse an uncompilable pattern at load:

FlagMatch.matches_regex is not a valid regex with flags=0: unterminated character set at position 1

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 flags: 99999999 case). Your diagnosis of why it had to move was the decisive part: the criterion could report, the rule surface could not.

For the second half — when the shim's rule evaluation does raise — respond() now returns the error and record() books it as rule_error, so the fault is no longer byte-identical to a clean no-match. cli_called fails the whole log on it, the way it already fails on the .error sentinel:

Recorder could not evaluate its response rules on 1 invocation(s), so the agent saw
fallback output the task did not describe. First: "TypeError('int' object is not iterable)"

I did not make select_rule continue past a faulting rule. Once one rule cannot be evaluated, the responses the agent saw were not the ones the task described, so the honest outcome is failing the log rather than scoring a partially-correct dispatch. Tested by corrupting a rendered shim — the only route left now that such a pattern cannot load.

Non-blocking

  1. Dead needs_value — deleted. argv_match.predicate_needs_value now states it is the only implementation and says why a pydantic-side twin is undesirable, rather than claiming a mirror. I did not widen CE037 to public model members: the false-positive surface (properties read only from templates and reports) looked wider than the defect class. Happy to be overruled.
  2. Untyped seam — now MatchSpec / FlagPredicate / ResponseRule TypedDicts, required keys indexed directly instead of .get(...) or <default>. Your ignore_flags rename now fails typechecking. Two tests pin that the TypedDict key sets equal the model field sets, which is what makes the single cast honest. record narrowed to dict[str, object] to match parse_log.
  3. One-directional parity — added assert set(CliMatch.model_fields) == set(MATCH_FACET_FIELDS). Your env mutation now fails.
  4. Shim invariants on one shape only — parametrized over both, and you were right that the spliced shape violated the ASCII one. argv_match.py is ASCII-only now, stated as a rule in its own docstring rather than left to luck.
  5. Shadowed rules accepted silently — now a load error, deliberately narrow, since "A matches everything B matches" is not decidable in general. Two sound cases: an exact duplicate, and a verb-only earlier rule whose verb prefixes a later one under the same flag parsing. Your value_flags example is exactly why the parsing clause is there — with different value_flags, a leading --folder F shifts the positionals and the earlier rule does not in fact claim the later one. Five reachable arrangements are pinned as still-accepted.

Nits

All taken: doubled paren fixed; MergeFieldField on responses with the test row dropped and the description corrected (you were right that RecordedCli is never a merge root, so it was inert metadata reading as a knob); CE047 now also reserves SHIM_GLOBALS and derives its target set from invocation_log.EMBEDDED_MODULES, with a test asserting it matches a file that exists; the guide's example uses a non-output flag and now says outright that flags: {output: ...} is the one thing not copy-pastable between the two surfaces.

BREAKING CHANGE footer added to dbc5afb, covering both the flag-in-verb rejection and the matches_regex move to load time.

Verification: ruff format, ruff check, pyright, and the lint suite are clean; 4677 pass. The same 8 failures as before are pre-existing and unrelated (Windows symlink privilege, float.numerator, and the CE033 drift test's own missing encoding=) — identical set at origin/main with this branch stashed.


from pydantic import BaseModel, ConfigDict, Field, model_validator

from coder_eval.argv_match import FlagPredicate, MatchSpec, is_number
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants