diff --git a/.claude/harness-candidates.md b/.claude/harness-candidates.md index e067e9ad..351df4a9 100644 --- a/.claude/harness-candidates.md +++ b/.claude/harness-candidates.md @@ -455,3 +455,28 @@ with the two `action.yml` items above — one considered change to the action's `final_status`, which does not exist in `run.json` and would have made a new assertion dead on arrival. Guard: assert the key set that non-Python consumers depend on, mirroring how CE030 pins doc/schema parity. +- [ ] **A probe task's detector must not be satisfiable from the sandbox-readable task + YAML.** `docker_runner._stage_inputs` serialises the post-override `TaskDefinition` to + `/work/input/task.yaml` and mounts `tasks/` again at `/work/task_dir`, both agent-readable. + Two probes now depend on NOT being satisfiable from that text — `anti_cheat_reference` via a + regex that cannot match its own source, `record_cli_responses` via a log-derived detector — + and nothing enforces it. `record_cli_responses` originally shipped (in review) with + `file_contains` needles that were verbatim in its own YAML, which would have let a + transcribing agent pass while dispatch was dead. Guard: for every `smoke-pass` task, assert no + `file_contains` needle / `file_matches_regex` pattern on a must-match criterion appears in the + serialised task YAML. Deferred: needs per-criterion-type handling and a real false-positive + pass (paths and generic words will collide), so well over 30 min. +- [ ] **A new shim failure mode must still RECORD the invocation.** A generated shim that dies + before `record()` leaves a log byte-identical to "the agent never ran it", which passes a + `max_count: 0` guard. The sidecar import was exactly that, caught only in review. Guard: + render each shim shape, break each external dependency in turn, assert the log is non-empty. + Deferred: "each external dependency" has no enumeration today, so the rule needs a seam + (a declared list of what a shim depends on) before it can be mechanical rather than a + hand-maintained list that decays. +- [ ] **A generated-artifact invariant must be asserted against a real run of that artifact, + not against the config that produced it.** `TestRecordCliProbeIntegrity` first shipped + comparing the task YAML with itself and hardcoding `"rule": 0` — a spelling `json.dumps`'s + default separators own — so a separator change would have left it green while the blocking CI + probe failed. Now fixed for this case by running a real shim. Deferred as a general guard: + "derives its expectation from the thing it checks" is not mechanically detectable; it belongs + in the review rubric rather than a lint rule. diff --git a/.github/workflows/pr-checks.yml b/.github/workflows/pr-checks.yml index 3bf52e59..58c450cd 100644 --- a/.github/workflows/pr-checks.yml +++ b/.github/workflows/pr-checks.yml @@ -469,16 +469,16 @@ jobs: AWS_BEARER_TOKEN_BEDROCK: ${{ secrets.AWS_BEARER_TOKEN_BEDROCK }} AWS_REGION: ${{ secrets.AWS_REGION }} BEDROCK_MODEL: ${{ secrets.BEDROCK_MODEL }} - # tasks_run for --tags smoke-pass. 7 task files (hello_date, dataset_example, + # tasks_run for --tags smoke-pass. 8 task files (hello_date, dataset_example, # smoke_llm_judge, smoke_agent_judge, byod_smoke_test, agentless_smoke_test, - # anti_cheat_reference); dataset_example fans out to 2 inline rows, so 8 - # sub-tasks. If you add/remove a smoke-pass task or change the dataset row - # count, bump these. + # anti_cheat_reference, record_cli_responses); dataset_example fans out to 2 + # inline rows, so 9 sub-tasks. If you add/remove a smoke-pass task or change + # the dataset row count, bump these. # # anti_cheat_reference lives in a SUBDIRECTORY, which `tasks/*.yaml` does not # match — the smoke-pass step names its path explicitly. Keep that in sync. - EXPECTED_SMOKE_PASS_RUN: "8" - EXPECTED_SMOKE_PASS_SUCCEEDED: "8" + EXPECTED_SMOKE_PASS_RUN: "9" + EXPECTED_SMOKE_PASS_SUCCEEDED: "9" # smoke-fail bucket: three tasks expected to fail. # 1. smoke_negative_path: file_contains criterion is unsatisfiable # (sentinel-string regression detection for success-checker). @@ -549,6 +549,9 @@ jobs: # explicitly. anti_cheat_reference is the adversarial probe that the agent # cannot read the reference solution during its turn; it needs the # coder-eval-agent image built above (it is a driver: docker task). + # record_cli_responses is the record_cli per-invocation-response probe and + # is also driver: docker, so it needs that same image; it is flat in + # tasks/, so the glob already matches it. - name: Run smoke-pass bucket (expect all to succeed) run: | .venv/bin/coder-eval run tasks/*.yaml tasks/anti_cheat_reference/*.yaml \ diff --git a/CLAUDE.md b/CLAUDE.md index 1e988f96..1664a9d2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,6 +27,8 @@ coder_eval/ ├── fs_permissions.py # set_permissions: stacked chmod window (via Sandbox.set_permissions) ├── pricing.py # Model pricing / cost calculation (ModelPricing, calculate_cost, register_pricing) ├── litellm_cost.py # Join proxy-captured ACTUAL per-call cost/cache onto turns (LiteLLM backend; apply_actual_cost) +├── invocation_log.py # record_cli recording shim: renders it (emitting a sibling import of the argv_match sidecar), and parse_log reads its JSON Lines back +├── argv_match.py # Structured argv matcher. STDLIB-ONLY (CE048): this file is copied into the recorder dir as a SIDECAR beside every response-serving shim, which imports it as a sibling, so `cli_called` and a `record_cli` response rule dispatch on ONE semantic ├── utils.py # Version info helpers │ ├── agents/ @@ -38,10 +40,11 @@ coder_eval/ │ ├── criteria.py # 15 success criterion types + base + union │ ├── experiment.py # ExperimentDefinition, ExperimentVariant, ResolvedTask, result models │ ├── judge_defaults.py # DEFAULT_JUDGE_MODEL constant (cycle-free leaf) +│ ├── cli_match.py # FlagMatch + CliMatch (a `when:` pattern) + the shared verb/flag validators (cycle-free leaf: criteria.py and sandbox.py both import it) │ ├── mutations.py # PromptMutation variants (prefix/suffix/replace/template) │ ├── results.py # CriterionResult (+ ClassificationCriterionResult), TurnRecord, EvaluationResult, EarlyStopInfo/EarlyStopReason, CriterionAggregate, ThresholdCheck, SuiteRollup │ ├── routing.py # ApiRoute (DirectRoute/BedrockRoute) -│ ├── sandbox.py # SandboxConfig, ResourceLimits +│ ├── sandbox.py # SandboxConfig, ResourceLimits, RecordedCli + CliResponse (per-invocation stub responses) │ ├── tasks.py # TaskDefinition, AgentConfig, Dataset (dataset fan-out + sample) │ ├── telemetry.py # CommandTelemetry, CommandStatistics, TokenUsage, ProviderCallCost, ReconciliationMessage, TranscriptMessage │ └── templates.py # RepoSource, TemplateDirSource, StarterFilesSource @@ -51,6 +54,7 @@ coder_eval/ │ ├── base.py # BaseCriterion (async _check_impl_async is primary; sync _check_impl derives from it, or vice versa) + @handle_criterion_errors(_async) │ ├── _classification_aggregate.py # Shared overlay: accuracy / P/R/F1 / confusion matrix │ ├── classification_match.py # File-based label matcher +│ ├── cli_called.py # Structured match over the record_cli invocation log (matching engine: argv_match.py) │ ├── command_executed.py │ ├── commands_efficiency.py │ ├── file_check.py @@ -216,11 +220,11 @@ make plugin-reference # the plugin's bundled criteria reference from the models Editing `src/coder_eval/pricing.py` means editing `evalboard/lib/pricing.ts` too — it is a hand-copied mirror, and `evalboard/lib/__tests__/pricing-parity.test.ts` fails the build on drift in either direction. -Recent additions, each traceable to a shipped defect: **CE037** (no unreferenced module-level private helper in `src/` — a helper whose docstring documents a bug the live code still has is worse than none), **CE038** (in an `@asynccontextmanager`, the acquire must sit INSIDE the `try` whose `finally` releases it — `asyncio.shield` protects the inner task, NOT the await, so a cancel on `__aenter__` skips the unwind while the work completes), **CE039** (a criterion checker must not return a gating `score=0.0` from an `except OSError` over a path the *task author* named — that books an eval-config error as an agent failure; raise `CheckerMisuseError` instead, and `# noqa: CE039` the cases that really are the agent's), **CE047** (every onboarding/marketing surface — README, `docs/index.md`, `docs/comparison.md`, `docs/llms.txt`, `mkdocs.yml`'s `site_description`, the Pages stub, and pyproject's `description`/`keywords` — must name every built-in `AgentKind`; OpenCode shipped while four of those seven still listed three harnesses, and nothing failed). +Recent additions, each traceable to a shipped defect: **CE037** (no unreferenced module-level private helper in `src/` — a helper whose docstring documents a bug the live code still has is worse than none), **CE038** (in an `@asynccontextmanager`, the acquire must sit INSIDE the `try` whose `finally` releases it — `asyncio.shield` protects the inner task, NOT the await, so a cancel on `__aenter__` skips the unwind while the work completes), **CE039** (a criterion checker must not return a gating `score=0.0` from an `except OSError` over a path the *task author* named — that books an eval-config error as an agent failure; raise `CheckerMisuseError` instead, and `# noqa: CE039` the cases that really are the agent's), **CE047** (every onboarding/marketing surface — README, `docs/index.md`, `docs/comparison.md`, `docs/llms.txt`, `mkdocs.yml`'s `site_description`, the Pages stub, and pyproject's `description`/`keywords` — must name every built-in `AgentKind`; OpenCode shipped while four of those seven still listed three harnesses, and nothing failed), **CE048** (a module copied into the recorder directory beside a generated sandbox shim — `models.sandbox.SIDECAR_MODULES`, currently `argv_match.py` — may import stdlib only. The failure is silent: the sidecar runs where `coder_eval` and its dependencies are not installed, so one package import makes every shadowed CLI die with an ImportError the agent reads as "the tool is broken", costing a whole run to diagnose. The rule derives its target set from that exported tuple and a test asserts it matches a file that exists — a lint rule guarding zero files must fail, not pass). When fixing a bug, ask: *could a custom lint rule have prevented this?* If the root cause is a mechanically detectable pattern (e.g., "always import from `coder_eval.models`", "never call blocking IO in async"), add a rule to `tests/lint/rules/` following the CE001+ pattern and wire it up in `tests/lint/runner.py`. This turns a one-time fix into permanent enforcement. See `tests/test_custom_lint.py` for how rules are tested. (Doc-surface / whole-tree rules that reason over Markdown/YAML or the entire `src/` tree rather than one `.py` AST at a time — CE026–CE031, CE033–CE036 — are not `BaseRule`s in the runner; they are wired as dedicated `@pytest.mark.lint` test classes. CE036 enforces the `live_verdict` determinism + monotonicity contract (`criteria/base.py`) that `EarlyStopWatcher`'s latching, deferred fail-stop, and flip-attribution silently depend on: monotonicity over arbitrary Python is undecidable, so instead of a static check it REPLAYS each live criterion against every prefix of recorded trajectories (`tests/lint/live_verdict_contract.py::CASES`) — on the authored ordering AND under seeded shuffles (`permuted_violations`, which catch order-sensitive bugs the authored walk misses) — and asserts the property directly, plus registry-derived coverage — every `LiveSuccessCriterion` in the union must have cases, and every polarity its instances claim via `live_decidable_polarities()` must actually be reached by one (otherwise a single always-`undecided` fixture would "cover" a type while proving nothing). Adding a live criterion therefore means adding `ContractCase`s in the same change. CE035 resolves every `steps..outputs.` / `needs..outputs.` reference in `.github/workflows/**` to a writer that actually produces that key — GitHub expands an unwritten output to the empty string, so a typo degrades a gate silently and actionlint models `steps.*.outputs` as an open string map. CE034 scans `tasks/` and forces an armed, live-*passable* `command_executed` to set `require_success` — a crashed invocation would otherwise latch a live PASS, fire `on_pass: stop`, and let FIRED-ONLY armed gating report SUCCESS without ever consulting the unarmed criteria (negative assertions are fail-only and are exempt). CE033 keeps the plugin's bundled `reference/criteria.md` in parity with the `SuccessCriterion` union that generates it (`make plugin-reference` writes it; the rule re-renders and diffs — never hand-edit the file). CE031 guards against dead config: a behavior-driving field on `SimulationConfig`/`RunLimits`/`Dataset` that no code reads by name. CE026 keeps the GitHub Action's onboarding surfaces honest — `README.md`, `docs/CI_GATE.md`, `docs/tutorials/02-ci-pipeline.md`, and the plugin's `ci` skill, whose emitted workflow users copy into their own repos: a page's *first* Action snippet must show the agent-runtime prerequisite steps (pinned to the `action-dogfood` job that proves them in CI), a zero-install absolute next to such a snippet must name the channel it means, every `github.com/marketplace/actions/` link plus the shields badge label must match `action.yml`'s `name:`, and every `with:` key on a snippet's action step must be a real `action.yml` input (GitHub ignores unknown inputs, so a rename would silently degrade every copied workflow). Renaming an action input or changing its runtime prerequisites therefore means updating the skill too.) -Adding a user-facing field to one of the models CE030 tracks (`TaskDefinition`, `RunLimits`, `Dataset`, `SimulationConfig` — see `tests/lint/doc_schema_parity.py`) means documenting it in its guide (mention the field name as inline code) or adding an `EXEMPT` entry with a reason it is not user-authored. `make lint` fails otherwise. +Adding a user-facing field to one of the models CE030 tracks (`TaskDefinition`, `RunLimits`, `Dataset`, `SimulationConfig`, `RecordedCli`, `CliResponse` — see `tests/lint/doc_schema_parity.py`, which is the SSOT; this list is a convenience copy) means documenting it in its guide (mention the field name as inline code) or adding an `EXEMPT` entry with a reason it is not user-authored. `make lint` fails otherwise. **Docs index SSOT.** `nav:` plus `extra.docs_index` (blurbs) in `mkdocs.yml` are the single source of truth for the flat index surfaces — `README.md`'s Documentation table, `docs/index.md`'s "Where to go next" table, and the `## Docs` / `## Tutorials` sections of `docs/llms.txt`. Regenerate all three with `make docs-indexes`; **CE028** fails the build if any drifts, if a nav page lacks a blurb (or vice-versa), or if a `docs/*.md` page is missing from the nav. The website sidebar derives from the same `nav:`. When adding or renaming a docs page, edit `nav:` + `extra.docs_index` and run `make docs-indexes` — never hand-edit the generated tables (they sit between `` / `` markers). diff --git a/docs/TASK_DEFINITION_GUIDE.md b/docs/TASK_DEFINITION_GUIDE.md index 57151805..0ad37c71 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -567,7 +567,41 @@ Notes: - **The log is seeded empty**, so a correct run that legitimately calls nothing still satisfies a `max_count: 0` guard — while a *missing* log (mock never ran, or wrote elsewhere) still fails. - **stdin is never read** by the shim: reading it would block whenever the sandbox leaves stdin attached to an open pipe, hanging the task. - **Collisions are rejected.** If a `mock_path_dirs` entry already provides an executable of the same name, setup raises rather than letting directory order decide which one runs. -- **It stubs a tool; it does not proxy one, and it does not serve per-invocation responses.** Recording a *real* executable on the way through, or returning different output per invocation, stays a hand-written mock under `mock_path_dirs` — both depend on state the harness cannot guarantee (the tool being installed, PATH order, live credentials, a fixture set). +- **It stubs a tool; it does not proxy one.** Recording a *real* executable on the way through stays a hand-written mock under `mock_path_dirs` — that depends on state the harness cannot guarantee (the tool being installed, PATH order, live credentials). + +#### Answering each invocation differently + +An agent whose next step depends on what the tool just told it cannot be evaluated by a stub that replies the same way to everything it types. `responses` gives one shadowed executable a reply per invocation: + +```yaml +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: {model: gemini_2_5_pro} + stdout: '{"id": "proj-1", "name": "Invoices"}' + - when: {verb: "ixp projects get missing"} + exit_code: 4 + stderr: "project not found\n" +``` + +- **`when` takes the same facets as [`cli_called`](#cli_called)** — `verb`, `verb_any_of`, `positional`, `flags`, `value_flags`, `ignore_flags` — evaluated by the same matcher, so the pattern that *serves* a response is the pattern that *grades* it. Always a mapping: a bare `when: "ixp dummy1"` is rejected (with the `{verb: ...}` spelling in the message), since a pattern has six facets and a lone string leaves which one you meant to infer. +- **First match wins**, in declaration order: put the specific rule above the general one. An invocation no rule claims gets the entry's own `exit_code` / `stdout` / `stderr`. +- **`exit_code` defaults to 0 on a rule** — the opposite of the entry default of 1. A rule exists because you described that invocation, so the natural reading is "and this is what it answers"; an undescribed one should still look like a tool that failed. +- **`ignore_flags` is empty on a rule**, unlike the criterion's `[output]`: grading must not depend on a flag that changes nothing about the outcome, but a rule may legitimately answer differently for `--output json`. That is the one place a rule is *not* copy-pastable into a criterion — `flags: {output: ...}` is valid on a rule and rejected on the criterion, which ignores that flag by default. +- **The log names the rule that answered** (`"rule": 1`), and omits the key when none did — the first thing you want to know when an expected canned response does not arrive. +- **The recorder directory also holds `argv_match.py`** — the matcher module the shim imports as a sibling, written there only for entries that declare `responses`. It is regenerated on every sandbox setup, so do not edit it, and do not declare a `tool` that would shadow it (the name is rejected). +- **A runnable worked example** ships in the repo: [`tasks/record_cli_responses.yaml`](https://github.com/UiPath/coder_eval/blob/main/tasks/record_cli_responses.yaml) stubs two subcommands with different replies, has the agent capture what each printed, and grades both the log and the captured text. Run it with `coder-eval run tasks/record_cli_responses.yaml` (needs `make docker-image` — it is a `driver: docker` task). +- **Still stateless.** A rule answers the same way however many times it matches; a counter would have to survive concurrent agent commands. For a tool whose reply must change over a run, hand-write a mock under `mock_path_dirs`. ## Template Sources @@ -973,6 +1007,8 @@ Use this instead of `command_executed` or `file_matches_regex` when a test shado Do **not** shorten the verb instead. `verb: "ixp projects"` matches all of its subcommands, so a positive assertion that the agent *read* a project is equally satisfied by `ixp projects delete`. Two entries are rejected when one prefixes the other, since the shorter already accepts everything the longer does. +**A verb holds subcommands only.** `verb: "ixp projects get --output json"` is rejected: the verb is compared against the *non-flag* arguments, so a flag inside it could never match — the criterion would score 0 against a log holding that exact call. Put it in `flags:` instead. (`head -1` still validates: a bare negative number is a value to the argument splitter, not a flag.) + **The argument tail stays open.** `positional` is a prefix too, so `verb: "ixp projects list"` with `positional: ["proj-1"]` also matches `ixp projects list proj-1 dummy`. To require a specific tail, name every argument in it. `positional: []` is rejected — it would assert nothing. **Declare value-bearing flags when you use `positional`.** An undeclared flag is treated as a switch, so its value stays among the non-flag arguments and shifts the ones you named. `get proj-1 --folder Finance` matches `positional: ["proj-1"]`, but `get --folder Finance proj-1` does **not** — `Finance` takes the first slot. Add `folder` to `value_flags` (or name it in `flags`) to fix it. Resolving the ambiguity this way is deliberate: guessing that an unknown flag consumes the next token let `--yes proj-1` bind `yes=proj-1` and swallow the project name, which made a `max_count: 0` delete guard pass on the delete it forbade. diff --git a/src/coder_eval/argv_match.py b/src/coder_eval/argv_match.py new file mode 100644 index 00000000..b929e580 --- /dev/null +++ b/src/coder_eval/argv_match.py @@ -0,0 +1,267 @@ +"""Structured argv matching: the one engine both CLI surfaces share. + +Two places ask the same question about one invocation. The ``cli_called`` +criterion reads a recorded ``argv`` back afterwards and asks *did this happen*; +a ``record_cli`` response rule asks it live, inside the sandbox, to choose which +canned response to serve. An author who writes ``verb: "ixp projects get"`` in a +rule and again in the criterion that grades it must get one semantic, not two +that drift. + +Everything here takes PLAIN DICTS rather than pydantic models, and imports +nothing beyond the standard library: ``Sandbox._generate_cli_recorders`` copies +this file into the recorder directory as a SIDECAR beside every shim that +declares response rules, and that shim imports it as a sibling while running +inside the sandbox, where ``coder_eval`` is not installed. Lint rule CE048 keeps +the imports stdlib-only. + +:class:`MatchSpec` is what ``CliMatch.match_spec`` emits. It is a ``TypedDict`` +rather than a bare dict on purpose: it is the seam where every guarantee the +pydantic models establish would otherwise be erased, and the fallback for a key +the reader failed to find is always "unconstrained" -- the direction that makes a +rule match everything, or a criterion score 1.0 against any log. TypedDict is +closed, so a key renamed on either side is a pyright error on both. +""" + +import re +from typing import TypedDict + + +class FlagPredicate(TypedDict): + """One ``FlagMatch``, lowered. Every key present: the producer dumps the model.""" + + 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): + """One authored pattern, lowered. ``verb_spellings`` is empty for no constraint. + + ``positional`` / ``flags`` are None when unconstrained -- None rather than + absent, so a reader indexes required keys directly and a missing one is a + KeyError rather than a silently wider match. + """ + + verb_spellings: list[list[str]] + positional: list[str] | None + flags: dict[str, FlagPredicate] | None + value_flags: list[str] + ignore_flags: list[str] + + +class ResponseRule(TypedDict): + """One ``CliResponse``, lowered -- what the shim carries and dispatches on.""" + + when: MatchSpec + exit: int + stdout: str + stderr: str + + +def split_flags( + argv: list[str], + ignore: frozenset[str], + value_flags: frozenset[str], + known_names: frozenset[str] = frozenset(), +) -> tuple[list[str], dict[str, list[str]]]: + """Split ``argv`` into non-flag arguments and a flag map. + + Only flags in ``value_flags`` consume a following token; everything else is a + switch. Guessing instead (``--yes proj-1`` binding ``yes=proj-1``) let a + ``max_count: 0`` guard pass on the delete it forbade, so ambiguity resolves + toward keeping the token positional. + + ``--flag=value`` binds directly, being unambiguous. Repeated flags accumulate. + ``ignore`` names are dropped with their values. ``--`` ends flag parsing and is + itself dropped; a lone ``-`` is positional. + + ``known_names`` are the flag names the spec mentions at all (including + presence predicates and aliases). A declared name is always taken whole, so a + genuine multi-char short flag still matches; undeclared ones are split. + """ + positional: list[str] = [] + flags: dict[str, list[str]] = {} + + def record(name: str, value: str) -> None: + if name not in ignore: + flags.setdefault(name, []).append(value) + + index = 0 + end_of_flags = False + while index < len(argv): + token = argv[index] + index += 1 + + if end_of_flags or not token.startswith("-") or token == "-": + positional.append(token) + continue + if token == "--": + end_of_flags = True + continue + + # Equals form: unambiguous, bind it and move on. + if "=" in token: + name, _, value = token.partition("=") + record(name.lstrip("-"), value) + continue + + name = token.lstrip("-") + known = name in value_flags or name in known_names + + # A bare negative number is a value, not a flag. Reading `-1` as a flag + # named `1` drops it from the positionals -- the same silent-disappearance + # that let `--yes proj-1` slip a delete past a guard. + if not known and is_number(name): + positional.append(token) + continue + + # Clustered short flags: `-rf` is `-r -f`. Declared names win, so a real + # multi-char short flag still matches, and `-fvalue` binds when `f` takes + # a value; otherwise each character is its own switch, which is what stops + # `-yf` escaping an `aliases: [y]` predicate. + if not known and not token.startswith("--") and len(name) > 1: + head, rest = name[0], name[1:] + if head in value_flags: + record(head, rest) + else: + for char in name: + record(char, "") + continue + + if name in value_flags and index < len(argv): + record(name, argv[index]) + index += 1 + else: + # Switch: empty value, and the next token is left for the positionals. + record(name, "") + + return positional, flags + + +def is_number(text: str) -> bool: + """Whether ``text`` parses as a number, so ``-1`` reads as a value not a flag.""" + try: + float(text) + except ValueError: + return False + return True + + +def predicate_needs_value(predicate: FlagPredicate) -> bool: + """Whether evaluating this flag predicate requires the flag's VALUE. + + Presence predicates (``present`` / ``absent``) do not, so they must not make + a flag value-bearing: asserting a boolean switch would otherwise make it + consume the following token, dropping that token from the positionals. That + is how `flags: {yes: {present: true}}` on a guard over `delete --yes proj-1` + once bound ``yes=proj-1``, dropped the project name, and handed the guard a + false PASS. + + The ONLY implementation of the rule. ``FlagMatch`` deliberately does not + carry a pydantic-side twin: two spellings of one predicate rule is how a rule + and the criterion grading it come to parse the same argv differently. + """ + return not (predicate["present"] or predicate["absent"]) + + +def flag_matches(predicate: FlagPredicate, values: list[str] | None) -> bool: + """Whether a recorded flag satisfies one flag predicate. + + ``values`` is None when the flag was not passed at all. Every non-``absent`` + predicate is satisfied by ANY of a repeated flag's values. + """ + if predicate["absent"]: + return values is None + if predicate["present"]: + return values is not None + if values is None: + return False + if (equals := predicate["equals"]) is not None: + return any(value == equals for value in values) + if (contains := predicate["contains"]) is not None: + return any(contains in value for value in values) + if (any_of := predicate["any_of"]) is not None: + allowed = set(any_of) + return any(value in allowed for value in values) + if (pattern := predicate["matches_regex"]) is not None: + # Compiled at load by FlagMatch, so this cannot raise on a spec the models + # produced -- and re caches, so recompiling per invocation is not a cost. + regex = re.compile(pattern, predicate["flags"]) + return any(regex.search(value) is not None for value in values) + # Unreachable: the model guarantees exactly one predicate. Raise rather than + # return False so a predicate added without a matcher arm here fails loudly. + raise AssertionError(f"flag predicate has no matcher arm: {predicate!r}") + + +def argv_matches(spec: MatchSpec, argv: list[str]) -> bool: + """Whether ``argv`` satisfies every configured facet of one match spec.""" + flag_specs = spec["flags"] or {} + + def names_of(flag: str, predicate: FlagPredicate) -> tuple[str, ...]: + return (flag, *predicate["aliases"]) + + # Declarations only. Folding `ignore_flags` into value_flags made ignored + # SWITCHES value-bearing, which swallowed the next positional and reopened a + # guard false-PASS; an ignored flag that takes a value declares it in + # value_flags. + ignore = frozenset(spec["ignore_flags"]) + value_flags = frozenset( + name + for flag, predicate in flag_specs.items() + if predicate_needs_value(predicate) + for name in names_of(flag, predicate) + ) | frozenset(spec["value_flags"]) + known_names = ( + frozenset(name for flag, predicate in flag_specs.items() for name in names_of(flag, predicate)) | ignore + ) + + positional, flags = split_flags(argv, ignore, value_flags, known_names) + + offset = 0 + spellings = spec["verb_spellings"] + if spellings: + # Token-wise, not a subset and not a string startswith: `labellings confirm` + # must never be satisfied by `labellings unconfirm`. Taking the first match is + # safe because validation rejects one spelling prefixing another, so no argv + # can match two. + matched = next((tokens for tokens in spellings if positional[: len(tokens)] == list(tokens)), None) + if matched is None: + return False + # Measured from the spelling that matched, since spellings can differ in length. + offset = len(matched) + + expected = spec["positional"] + if expected is not None and positional[offset : offset + len(expected)] != list(expected): + return False + + for flag, predicate in flag_specs.items(): + # [] means absent under every spelling, which flag_matches distinguishes + # from a switch's "present with empty value" ([""]). + collected = [value for name in names_of(flag, predicate) for value in flags.get(name, [])] + if not flag_matches(predicate, collected or None): + return False + + return True + + +def select_rule(rules: list[ResponseRule], argv: list[str]) -> tuple[int, ResponseRule] | None: + """``(index, rule)`` of the first rule whose ``when`` spec matches ``argv``, or None. + + First match wins, so ordering is the author's disambiguation tool: the + specific rule goes above the general one. Stateless by design -- the same + argv gets the same answer every time, which keeps the shim free of on-disk + counters that two concurrent agent commands would race on. + + The index travels with the rule because the shim records it: "no rule + matched" and "a rule matched and looks like the default" are otherwise the + same line in the log. + """ + for index, rule in enumerate(rules): + if argv_matches(rule["when"], argv): + return index, rule + return None diff --git a/src/coder_eval/criteria/cli_called.py b/src/coder_eval/criteria/cli_called.py index 55811e6d..1f251602 100644 --- a/src/coder_eval/criteria/cli_called.py +++ b/src/coder_eval/criteria/cli_called.py @@ -1,13 +1,13 @@ """CLI-called criterion checker — structured matching over an invocation log.""" import logging -import re import shlex -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING +from coder_eval.argv_match import argv_matches from coder_eval.criteria.base import BaseCriterion, CheckContext, register_criterion from coder_eval.invocation_log import parse_log -from coder_eval.models import CliCalledCriterion, CriterionResult, FlagMatch +from coder_eval.models import CliCalledCriterion, CriterionResult if TYPE_CHECKING: @@ -17,165 +17,16 @@ logger = logging.getLogger(__name__) -def _split_flags( - argv: list[str], - ignore: frozenset[str], - value_flags: frozenset[str], - known_names: frozenset[str] = frozenset(), -) -> tuple[list[str], dict[str, list[str]]]: - """Split ``argv`` into non-flag arguments and a flag map. +def _record_matches(criterion: CliCalledCriterion, argv: list[str], record: dict[str, object]) -> bool: + """Whether one log record satisfies every configured facet of the criterion. - Only flags in ``value_flags`` consume a following token; everything else is a - switch. Guessing instead (``--yes proj-1`` binding ``yes=proj-1``) let a - ``max_count: 0`` guard pass on the delete it forbade, so ambiguity resolves - toward keeping the token positional. - - ``--flag=value`` binds directly, being unambiguous. Repeated flags accumulate. - ``ignore`` names are dropped with their values. ``--`` ends flag parsing and is - itself dropped; a lone ``-`` is positional. - - ``known_names`` are the flag names the criterion mentions at all (including - presence predicates and aliases). A declared name is always taken whole, so a - genuine multi-char short flag still matches; undeclared ones are split. - """ - positional: list[str] = [] - flags: dict[str, list[str]] = {} - - def record(name: str, value: str) -> None: - if name not in ignore: - flags.setdefault(name, []).append(value) - - index = 0 - end_of_flags = False - while index < len(argv): - token = argv[index] - index += 1 - - if end_of_flags or not token.startswith("-") or token == "-": - positional.append(token) - continue - if token == "--": - end_of_flags = True - continue - - # Equals form: unambiguous, bind it and move on. - if "=" in token: - name, _, value = token.partition("=") - record(name.lstrip("-"), value) - continue - - name = token.lstrip("-") - known = name in value_flags or name in known_names - - # A bare negative number is a value, not a flag. Reading `-1` as a flag - # named `1` drops it from the positionals -- the same silent-disappearance - # that let `--yes proj-1` slip a delete past a guard. - if not known and _is_number(name): - positional.append(token) - continue - - # Clustered short flags: `-rf` is `-r -f`. Declared names win, so a real - # multi-char short flag still matches, and `-fvalue` binds when `f` takes - # a value; otherwise each character is its own switch, which is what stops - # `-yf` escaping an `aliases: [y]` predicate. - if not known and not token.startswith("--") and len(name) > 1: - head, rest = name[0], name[1:] - if head in value_flags: - record(head, rest) - else: - for char in name: - record(char, "") - continue - - if name in value_flags and index < len(argv): - record(name, argv[index]) - index += 1 - else: - # Switch: empty value, and the next token is left for the positionals. - record(name, "") - - return positional, flags - - -def _is_number(text: str) -> bool: - try: - float(text) - except ValueError: - return False - return True - - -def _flag_matches(predicate: FlagMatch, values: list[str] | None) -> bool: - """Whether a recorded flag satisfies one :class:`FlagMatch` predicate. - - ``values`` is None when the flag was not passed at all. Every non-``absent`` - predicate is satisfied by ANY of a repeated flag's values. + ``tool`` is checked here rather than in :func:`argv_matches` because it is a + property of the RECORD, not of the arguments -- the shim that serves a + response knows which tool it is before it looks at argv. """ - if predicate.absent: - return values is None - if predicate.present: - return values is not None - if values is None: - return False - if predicate.equals is not None: - return any(value == predicate.equals for value in values) - if predicate.contains is not None: - return any(predicate.contains in value for value in values) - if predicate.any_of is not None: - allowed = set(predicate.any_of) - return any(value in allowed for value in values) - if predicate.matches_regex is not None: - regex = re.compile(predicate.matches_regex, predicate.flags) - return any(regex.search(value) is not None for value in values) - # Unreachable: FlagMatch guarantees exactly one predicate. Raise rather than - # return False so a predicate added without a matcher arm here fails loudly. - raise AssertionError(f"FlagMatch has no matcher arm: {predicate!r}") - - -def _record_matches(criterion: CliCalledCriterion, argv: list[str], record: dict[str, Any]) -> bool: - """Whether one log record satisfies every configured facet of the criterion.""" if criterion.tool is not None and record.get("tool") != criterion.tool: return False - - # Declarations only. Folding `ignore_flags` in here made ignored SWITCHES - # value-bearing, which swallowed the next positional and reopened the guard - # false-PASS; an ignored flag that takes a value declares it in value_flags. - positional, flags = _split_flags( - argv, - frozenset(criterion.ignore_flags), - frozenset(n for name, p in (criterion.flags or {}).items() if p.needs_value for n in (name, *p.aliases)) - | frozenset(criterion.value_flags), - frozenset(n for name, p in (criterion.flags or {}).items() for n in (name, *p.aliases)) - | frozenset(criterion.ignore_flags), - ) - - offset = 0 - spellings = criterion.verb_spellings - if spellings: - # Token-wise, not a subset and not a string startswith: `labellings confirm` - # must never be satisfied by `labellings unconfirm`. Taking the first match is - # safe because validation rejects one spelling prefixing another, so no argv - # can match two. - matched = next((tokens for tokens in spellings if positional[: len(tokens)] == tokens), None) - if matched is None: - return False - # Measured from the spelling that matched, since spellings can differ in length. - offset = len(matched) - - if criterion.positional is not None: - expected = criterion.positional - if positional[offset : offset + len(expected)] != expected: - return False - - if criterion.flags: - for name, predicate in criterion.flags.items(): - # [] means absent under every spelling, which _flag_matches - # distinguishes from a switch's "present with empty value" ([""]). - collected = [v for n in (name, *predicate.aliases) for v in flags.get(n, [])] - if not _flag_matches(predicate, collected or None): - return False - - return True + return argv_matches(criterion.match_spec, argv) @register_criterion @@ -202,21 +53,10 @@ def _check_impl( Result with binary score (1.0 when the match count is within [min_count, max_count], 0.0 otherwise) """ - # Up front so a bad pattern names its flag, rather than surfacing as a - # generic caught exception when some record first reaches that predicate. - for name, predicate in (criterion.flags or {}).items(): - if predicate.matches_regex is None: - continue - try: - re.compile(predicate.matches_regex, predicate.flags) - except (re.error, ValueError) as exc: - return CriterionResult( - criterion_type=criterion.type, - description=criterion.description, - score=0.0, - error=f"Invalid matches_regex for flag '{name}': {exc}", - ) - + # No pre-flight re.compile here: `FlagMatch` compiles the pattern at + # validation, so an uncompilable one never reaches a checker -- and it has + # to be caught there, because the response-rule surface that shares this + # model cannot report an error at all. if not sandbox.file_exists(criterion.log): # Harness fault, not agent behaviour. Failing stops a max_count: 0 # guard passing vacuously against a log that never existed. @@ -246,6 +86,72 @@ def _check_impl( usable, unusable = parse_log(content) + # Both fault checks below are scoped to the records this criterion is about. + # One log serves every shadowed tool, so a `uip` shim that could not import + # its matcher must not fail a `tool: curl` guard that has nothing to do with + # response dispatch -- and whose error message would not explain why. + mine = [record for _, record in usable if criterion.tool is None or record.get("tool") == criterion.tool] + + # Booked on every record when the shim could not IMPORT its matcher, so + # no rule was ever tried and the agent saw the entry defaults throughout. + # Scored 0.0 rather than raised, unlike `rule_error` below: the sidecar + # lives in the agent-writable recorder directory, so an agent can cause + # this, and escalating would hand it a way to turn a failing run into an + # ERROR. The records themselves are still trustworthy -- the shim keeps + # logging -- which is what stops a `max_count: 0` guard passing on a + # forbidden call that would otherwise have gone unrecorded entirely. + broken = [record for record in mine if record.get("sidecar_error") is not None] + if broken: + return CriterionResult( + criterion_type=criterion.type, + description=criterion.description, + score=0.0, + error=( + f"Recorder could not import its matcher module on {len(broken)} invocation(s), so no " + f"response rule was tried and the agent saw the entry defaults throughout. " + f"First: {broken[0].get('sidecar_error')!r}" + ), + ) + + # The shim books this when its own rule evaluation RAISED. If it fires, the + # responses the agent saw were not the ones the task described, so no verdict + # over this log means anything. + # + # ALL FIVE of this checker's refuse-to-score paths are uniform at a gating + # 0.0, and that uniformity is the point: + # + # missing log an agent can `rm` it + # write sentinel an agent can fill the disk or chmod the dir + # sidecar_error an agent can delete the matcher beside the shim + # unusable records an agent can append garbage to the log + # rule_error an agent can append a crafted record, or edit the shim + # + # EVERY one is agent-REACHABLE, because the whole recorder directory lives + # inside the sandbox the agent writes to. So none of them may raise: an + # escalation here is a `FinalStatus.ERROR`, which is normally read as "harness + # broken, discard this data point", and that is a strictly better outcome for + # a failing agent than FAILED. An earlier revision raised `CheckerMisuseError` + # on `rule_error` believing only a task author could cause it; appending one + # line to `calls.jsonl` disproved that. + # + # The legitimate concern that motivated the escalation -- a task author's + # unevaluable response spec must not be booked as an agent failure -- is + # handled where the agent cannot reach it instead: `RecordedCli` proves every + # rule is evaluable at LOAD time (see `_validate_responses_are_evaluable`), so + # an authoring mistake is a validation error before the sandbox even exists. + faults = [record for record in mine if record.get("rule_error") is not None] + if faults: + return CriterionResult( + criterion_type=criterion.type, + description=criterion.description, + score=0.0, + error=( + f"Recorder could not evaluate its response rules on {len(faults)} invocation(s), so the " + f"agent saw fallback output the task did not describe. The log cannot be trusted. " + f"First: {faults[0].get('rule_error')!r}" + ), + ) + if unusable: # A record we cannot read might BE the call a max_count: 0 guard # forbids, so scoring it "did not match" would let the guard pass. diff --git a/src/coder_eval/invocation_log.py b/src/coder_eval/invocation_log.py index 40228f57..420b5525 100644 --- a/src/coder_eval/invocation_log.py +++ b/src/coder_eval/invocation_log.py @@ -6,7 +6,12 @@ The rendered script runs INSIDE the sandbox, where ``coder_eval`` is not installed, so it imports nothing from this package: its configuration arrives as -embedded literals and everything else comes from the standard library. +literals, and everything else comes from the standard library. The one exception +is the argv matcher that dispatches its per-invocation responses -- copied into +the recorder directory as a SIDECAR module beside the shim +(:mod:`coder_eval.argv_match`, stdlib-only for exactly that reason) and imported +as a sibling, so the shim dispatches on the very module the ``cli_called`` +criterion grades with. Keeping the template here rather than inline in :mod:`coder_eval.sandbox` lets :func:`render_recorder` be exercised directly (render, execute, read the log) @@ -15,13 +20,17 @@ import json import sys +from importlib import resources -from coder_eval.models import RecordedCli +from coder_eval.models import RECORD_CLI_LOG_NAME, SIDECAR_MODULES, RecordedCli -# Written beside the shims, inside the generated recorder directory, so the log -# travels with them if the sandbox root moves. -LOG_FILENAME = "calls.jsonl" +# The shim imports exactly one of them -- the argv matcher. A second sidecar +# would need its own import line, so this unpacks rather than indexing: adding +# one is then a loud failure here instead of a silently un-imported file. +(_SIDECAR_MODULE,) = SIDECAR_MODULES +_SIDECAR_MODULE_STEM = _SIDECAR_MODULE.removesuffix(".py") + _TEMPLATE = '''\ #!{interpreter} @@ -32,6 +41,7 @@ every sandbox setup. """ +import importlib.util import json import os import sys @@ -41,13 +51,19 @@ EXIT_CODE = {exit_code!r} STDOUT_TEXT = {stdout!r} STDERR_TEXT = {stderr!r} +# Per-invocation responses in declaration order, empty when the entry declared +# none -- in which case every invocation gets the three defaults above. +RULES = {rules!r} SHIM_DIR = os.path.dirname(os.path.abspath(__file__)) +# Set by the sidecar import block below when the matcher could not be imported. +SIDECAR_ERROR = None +{sidecar_import} LOG_PATH = os.path.join(SHIM_DIR, {log_filename!r}) LOG_ERROR_PATH = LOG_PATH + ".error" -def record(argv, exit_code): +def record(argv, exit_code, rule, rule_error): """Append this invocation to the log. Best-effort: a logging failure must never break the command the agent ran, @@ -58,6 +74,25 @@ def record(argv, exit_code): exists instead of a flattened command line. stdin is deliberately never read: it would block whenever the sandbox leaves it on an open pipe, and in passthrough mode it would consume the payload the real tool needs. + + `rule` is the index of the response rule that answered, recorded only when + one did. Without it, "no rule matched, so this is the default" and "rule 2 + answered, and happens to look like the default" are indistinguishable in the + log -- the first question asked when an expected canned response does not + arrive. + + `rule_error` is booked when rule evaluation RAISED, and it is what stops an + eval-config fault from reading as a clean no-match: the agent got fallback + output the task never described, so `cli_called` fails the whole log on it + rather than scoring a run whose responses were wrong. + + `sidecar_error` is the same idea one step earlier: the matcher module beside + this script could not be IMPORTED, so no rule could be tried at all. It is + booked on every record rather than returned per invocation, because the + import happens once at startup and fails for the whole process. Recording + the call anyway is the point -- a shim that answers and logs NOTHING is + indistinguishable from a tool the agent never ran, which is how a + `max_count: 0` guard over a forbidden call once scored a silent pass. """ entry = {{ "ts": round(time.time(), 3), @@ -65,6 +100,12 @@ def record(argv, exit_code): "argv": list(argv), "exit": exit_code, }} + if rule is not None: + entry["rule"] = rule + if rule_error is not None: + entry["rule_error"] = rule_error + if SIDECAR_ERROR is not None: + entry["sidecar_error"] = SIDECAR_ERROR try: # ensure_ascii escapes non-ASCII and any stray surrogate from # undecodable argv bytes, so an exotic argument cannot make this write @@ -82,20 +123,51 @@ def record(argv, exit_code): pass +def respond(argv): + """Pick this invocation's (exit code, stdout, stderr, rule index, rule error). + + First matching rule wins; whatever no rule claims gets the defaults. The + sidecar import above is emitted only when RULES is non-empty, so the RULES + half of this guard is what keeps `select_rule` from being NAMED when it was + never imported -- and the `select_rule is None` half covers the import being + emitted but having failed, which `record` reports via SIDECAR_ERROR. + """ + if not RULES or select_rule is None: + return EXIT_CODE, STDOUT_TEXT, STDERR_TEXT, None, None + try: + selected = select_rule(RULES, list(argv)) + except Exception as exc: + # Best-effort, like the log write: a matcher fault must not turn the stub + # into a crashing executable, which the agent would read as the tool + # itself breaking in a way the task never described. Unlike the log + # write it is also RETURNED, so the record says what happened -- an + # untraceable fallback here scores the task as if the agent had never + # made the call at all. + sys.stderr.write("coder_eval recorder: response matching failed: %r\\n" % (exc,)) + return EXIT_CODE, STDOUT_TEXT, STDERR_TEXT, None, repr(exc) + if selected is None: + return EXIT_CODE, STDOUT_TEXT, STDERR_TEXT, None, None + index, rule = selected + return rule["exit"], rule["stdout"], rule["stderr"], index, None + + def main(argv): - """Record the invocation, then fail like the tool would with nothing behind it. + """Answer the invocation from the canned responses, and record what happened. - Nothing is executed: no network, no auth, no side effects. A test that needs + Nothing is executed: no network, no auth, no side effects -- a response is + text the task author wrote, never the real tool's output. A test that needs the real tool's behavior recorded instead should supply its own wrapper under - mock_path_dirs -- proxying a live executable is a different job from stubbing + mock_path_dirs: proxying a live executable is a different job from stubbing one, and this shim deliberately does only the second. """ - record(argv[1:], EXIT_CODE) - if STDOUT_TEXT: - sys.stdout.write(STDOUT_TEXT) - if STDERR_TEXT: - sys.stderr.write(STDERR_TEXT) - return EXIT_CODE + args = argv[1:] + exit_code, stdout_text, stderr_text, rule, rule_error = respond(args) + record(args, exit_code, rule, rule_error) + if stdout_text: + sys.stdout.write(stdout_text) + if stderr_text: + sys.stderr.write(stderr_text) + return exit_code if __name__ == "__main__": @@ -103,6 +175,73 @@ def main(argv): ''' +# Rendered into the shim only when the entry declares rules. Every line of the +# comment is addressed at whoever opens a generated shim inside a sandbox, which +# is why the reasoning lives in the emitted text rather than only here. +# +# The module name is derived from SIDECAR_MODULES rather than written out, so +# renaming the sidecar cannot leave this import pointing at a file that no +# longer exists -- the one failure the write side would not catch. +_SIDECAR_IMPORT = f"""\ +# {_SIDECAR_MODULE} is written beside this shim by coder_eval SandboxConfig.record_cli. +# Loaded by ABSOLUTE PATH, not by name: a plain `import {_SIDECAR_MODULE_STEM}` resolves +# through sys.path, so an unrelated {_SIDECAR_MODULE_STEM} earlier on it (PYTHONPATH, the +# cwd, site-packages) would win over the file written beside this shim -- silently +# on a module that happens to export select_rule. +# +# This directory is dropped from sys.path first, and deliberately NOT re-added: +# it is agent-writable and holds one file per shadowed tool, so a tool named +# `typing.py` here would shadow the matcher's OWN stdlib imports (which still +# resolve through sys.path while it executes) and break every rules-bearing shim +# in the sandbox. Comparison is by realpath because sys.path[0] is resolved +# while SHIM_DIR, from abspath(__file__), is not. +# +# Bytecode is off first, so loading the sidecar cannot leave a __pycache__/ +# directory here for a file_check criterion or an artifact diff to trip over. +sys.dont_write_bytecode = True +_here = os.path.realpath(SHIM_DIR) +sys.path[:] = [_p for _p in sys.path if os.path.realpath(_p or ".") != _here] +try: + # A private module name, so this never collides in sys.modules with a real + # {_SIDECAR_MODULE_STEM} the sandbox may legitimately have installed. + _spec = importlib.util.spec_from_file_location( + "_coder_eval_{_SIDECAR_MODULE_STEM}", os.path.join(SHIM_DIR, {_SIDECAR_MODULE!r}) + ) + if _spec is None or _spec.loader is None: + raise ImportError("could not build a module spec for the argv matcher sidecar") + _sidecar = importlib.util.module_from_spec(_spec) + _spec.loader.exec_module(_sidecar) + select_rule = _sidecar.select_rule +except Exception as _exc: + # Never fatal: a shim that dies here is a tool that is ON PATH, answers + # nothing, and RECORDS NOTHING -- byte-identical in the log to a call the + # agent never made, which is how `max_count: 0` over a forbidden call scored + # a silent pass. Fall back to the entry defaults and let `record` book the + # fault on every line so the log is untrustworthy rather than empty. + select_rule = None + SIDECAR_ERROR = repr(_exc) +""" + + +def sidecar_source(module: str) -> str: + """The source of one sidecar module, to write beside a generated shim. + + Read as a package resource rather than copied off the filesystem, so a + zipimported install still works -- and never reconstructed or + re-implemented: the shim must dispatch on the SAME matcher the + ``cli_called`` criterion grades with, and every transformation in between is + a place the two could diverge. + + Raises: + ValueError: ``module`` is not a declared sidecar. The name is joined onto + the package directory, so an unvetted one reads an arbitrary module + (or escapes the package via ``..``). + """ + if module not in SIDECAR_MODULES: + raise ValueError(f"{module!r} is not a declared sidecar module; expected one of {SIDECAR_MODULES}") + return resources.files("coder_eval").joinpath(module).read_text(encoding="utf-8") + + def render_recorder(spec: RecordedCli, interpreter: str | None = None) -> str: """Render the shim source for one ``record_cli`` entry. @@ -110,14 +249,30 @@ def render_recorder(spec: RecordedCli, interpreter: str | None = None) -> str: the running interpreter). A ``#!/usr/bin/env python3`` shebang resolves through the same PATH the recorder dir is prepended to, so `tool: python3` made the shim re-exec itself forever. + + The sidecar import is emitted only when the entry declares ``responses``: a + shim that answers every invocation the same way never consults the matcher, + and leaving the import out keeps the common shim runnable on its own, with + no sibling file to write. """ + rules = [ + { + "when": response.when.match_spec, + "exit": response.exit_code, + "stdout": response.stdout, + "stderr": response.stderr, + } + for response in spec.responses + ] return _TEMPLATE.format( interpreter=interpreter or sys.executable, tool=spec.tool, exit_code=spec.exit_code, stdout=spec.stdout, stderr=spec.stderr, - log_filename=LOG_FILENAME, + rules=rules, + sidecar_import=_SIDECAR_IMPORT if rules else "", + log_filename=RECORD_CLI_LOG_NAME, ) diff --git a/src/coder_eval/models/__init__.py b/src/coder_eval/models/__init__.py index 97d4f64c..be05a859 100644 --- a/src/coder_eval/models/__init__.py +++ b/src/coder_eval/models/__init__.py @@ -20,6 +20,12 @@ parse_agent_config, ) +# Argv matching (shared by cli_called and record_cli response rules) +from coder_eval.models.cli_match import ( + CliMatch, + FlagMatch, +) + # Container paths (leaf constants; re-exported so consumers obey CE001) from coder_eval.models.container_paths import ( CONTAINER_INPUT_DIR, @@ -47,7 +53,6 @@ FileContainsCriterion, FileExistsCriterion, FileMatchesRegexCriterion, - FlagMatch, JMESPathAssertion, JsonCheckCriterion, LivePolarity, @@ -165,6 +170,9 @@ from coder_eval.models.sandbox import ( RECORD_CLI_DIR, RECORD_CLI_LOG, + RECORD_CLI_LOG_NAME, + SIDECAR_MODULES, + CliResponse, DockerBuildConfig, DockerDriverConfig, NodeEnvConfig, @@ -252,6 +260,8 @@ "ReferenceComparisonCriterion", "CommandExecutedCriterion", "CliCalledCriterion", + "CliMatch", + "CliResponse", "FlagMatch", "CommandsEfficiencyCriterion", "UiPathEvalCriterion", @@ -298,6 +308,8 @@ "RecordedCli", "RECORD_CLI_DIR", "RECORD_CLI_LOG", + "RECORD_CLI_LOG_NAME", + "SIDECAR_MODULES", "ResourceLimits", "validate_template_sources_list", # Telemetry diff --git a/src/coder_eval/models/cli_match.py b/src/coder_eval/models/cli_match.py new file mode 100644 index 00000000..b65fc96f --- /dev/null +++ b/src/coder_eval/models/cli_match.py @@ -0,0 +1,392 @@ +"""Argv-matching models shared by ``cli_called`` and ``record_cli`` response rules. + +A cycle-free leaf, like :mod:`coder_eval.models.judge_defaults`: both +:mod:`coder_eval.models.criteria` and :mod:`coder_eval.models.sandbox` import it, +and sandbox.py could not import from criteria.py in any case (criteria.py already +takes ``RECORD_CLI_LOG`` from sandbox.py). + +The matching *semantics* live in :mod:`coder_eval.argv_match`, which is +stdlib-only because it is copied beside every generated shim that serves +per-invocation responses and imported there as a sibling. This module +holds the authoring surface — the pydantic models and the validators that reject +a pattern which cannot mean what it looks like — and lowers it to the plain spec +dict that engine consumes. +""" + +from __future__ import annotations + +import itertools +import re +from typing import Any, cast + +from pydantic import BaseModel, ConfigDict, Field, model_validator + +from coder_eval.argv_match import FlagPredicate, MatchSpec, is_number + + +class FlagMatch(BaseModel): + """Predicate for ONE flag value. + + Exactly one predicate field may be set. In YAML a bare scalar is accepted as + shorthand for ``equals`` (``model: gemini_2_5_pro`` == ``model: {equals: + gemini_2_5_pro}``), which keeps the common case unnested. + + ``absent: true`` asserts the flag was NOT passed — distinct from "passed with + a different value", and the reason this is a predicate rather than a bare + ``dict[str, str]`` on the criterion. + + The one-predicate rule means a conjunction on a single flag ("contains BOTH + A and B") is not expressible here. Either declare two ``cli_called`` criteria + over the same log, or use one ``matches_regex`` that spans both — the latter + is what a heredoc-built JSON payload usually wants, together with + ``flags: 16`` (``re.DOTALL``) so ``.`` crosses the payload's newlines. + """ + + model_config = ConfigDict(extra="forbid") + + equals: str | None = Field(default=None, description="Flag value must equal this string exactly") + contains: str | None = Field(default=None, description="Flag value must contain this substring") + matches_regex: str | None = Field( + default=None, + description="Flag value must match this regex. Scoped to ONE value, unlike a whole-line pattern", + ) + any_of: list[str] | None = Field( + default=None, + min_length=1, + description=( + "Flag value must equal one of these strings. Non-empty: an empty list would match " + "nothing, so a max_count: 0 guard built on it would pass vacuously" + ), + ) + absent: bool = Field(default=False, description="Flag must NOT be present in the invocation") + present: bool = Field( + default=False, + description=( + "Flag must be present, whatever its value -- the predicate for a boolean switch. Unlike " + '`equals: ""` it survives a CLI that spells the switch `--force true`, and it never makes ' + "the flag value-bearing, so asserting a switch cannot swallow the next positional" + ), + ) + aliases: list[str] = Field( + default_factory=list, + description=( + "Other names for the SAME flag, e.g. aliases: [y] on a `yes` predicate so `-y` and " + "`--yes` are one flag. Values are gathered across every name: `present` holds if any " + "appeared, `absent` only if none did, a value predicate matches if any value under any " + "name satisfies it" + ), + ) + flags: int = Field( + default=0, + description=( + "Regex flags for matches_regex (re.IGNORECASE=2, re.MULTILINE=8, re.DOTALL=16), " + "mirroring FileMatchesRegexCriterion.flags. DOTALL is the usual need, since a " + "heredoc-built flag value spans lines" + ), + ) + + @model_validator(mode="before") + @classmethod + def _coerce_scalar_shorthand(cls, value: Any) -> Any: + """Accept ``model: gemini_2_5_pro`` as ``model: {equals: ...}``.""" + if isinstance(value, str): + return {"equals": value} + return value + + @model_validator(mode="after") + def _exactly_one_predicate(self) -> FlagMatch: + set_predicates = [ + name for name in ("equals", "contains", "matches_regex", "any_of") if getattr(self, name) is not None + ] + if self.absent: + set_predicates.append("absent") + if self.present: + set_predicates.append("present") + if len(set_predicates) != 1: + msg = ( + "FlagMatch requires exactly one of equals / contains / matches_regex / any_of / absent / present, " + f"got {sorted(set_predicates) or 'none'}" + ) + raise ValueError(msg) + # `flags` only reaches re.compile via matches_regex; setting it beside any + # other predicate is a silent no-op, so reject it rather than mislead. + 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}" + raise ValueError(msg) + # Compile HERE, not in a checker: this model now feeds two consumers, and + # only one of them can report. A `record_cli` response rule evaluates the + # pattern inside the sandbox, where a PatternError is swallowed and the + # tool serves its fallback -- a log line indistinguishable from a + # legitimate no-match, so the task scores differently for identical agent + # behaviour with nothing on any report surface. At load, both surfaces + # refuse the pattern instead. + if self.matches_regex is not None: + try: + re.compile(self.matches_regex, self.flags) + except (re.error, ValueError) as exc: + msg = f"FlagMatch.matches_regex is not a valid regex with flags={self.flags}: {exc}" + raise ValueError(msg) from exc + return self + + +# The argv facets every matching surface must offer. `cli_called` declares these +# fields itself (with grading-specific guidance in each description) rather than +# inheriting them, so a facet added to one surface and forgotten on the other is +# caught by the parity test in tests/test_cli_match_parity.py instead of shipping +# as a rule the criterion cannot express. +MATCH_FACET_FIELDS: tuple[str, ...] = ("verb", "verb_any_of", "positional", "flags", "value_flags", "ignore_flags") + + +def verb_spellings_of(verb: str | None, verb_any_of: list[str] | None) -> list[list[str]]: + """Each accepted verb as its token list; empty when there is no verb constraint. + + The only place either verb field is split, so the validators, the matcher and + the failure detail cannot disagree. + """ + if verb is not None: + return [verb.split()] + if verb_any_of is not None: + return [spelling.split() for spelling in verb_any_of] + return [] + + +def validate_verbs(verb: str | None, verb_any_of: list[str] | None, spellings: list[list[str]], label: str) -> None: + """Reject verb declarations that cannot mean what they look like. + + ``label`` names the surface (``cli_called``, ``record_cli response when``) so + the message points at the block the author actually wrote. + """ + if verb is not None and verb_any_of is not None: + msg = f"{label} accepts verb or verb_any_of, not both" + raise ValueError(msg) + # Falsy, so an at-least-one-facet check would read it as "no verb". + if verb_any_of is not None and not verb_any_of: + msg = f"{label} verb_any_of must not be empty: drop the field to match any verb" + raise ValueError(msg) + # A character count would pass " ", whose split() is an empty prefix. + if any(not tokens for tokens in spellings): + msg = f"{label} verb must not be blank: a blank verb is an empty prefix and matches every invocation" + raise ValueError(msg) + # A verb is compared against the NON-FLAG arguments, so a flag written into it + # can never match anything -- and the failure is silent: the criterion scores 0 + # against a log that holds the very call it describes, and a response rule falls + # through to the tool's default. Inviting, too, since a whole verb reads like a + # command line. `is_number` mirrors the splitter's own rule so this check cannot + # forbid a token (`-1`) that the matcher would in fact have seen. + for tokens in spellings: + for token in tokens: + if token.startswith("-") and token != "-" and not is_number(token.lstrip("-")): + msg = ( + f"{label} verb token {token!r} looks like a flag. A verb matches only the " + "non-flag arguments, so a flag inside it can never match. Put it in `flags:` " + f"instead, e.g. flags: {{{token.lstrip('-').split('=')[0]}: }}." + ) + raise ValueError(msg) + for first, second in itertools.combinations(spellings, 2): + if first == second: + msg = f"{label} verb_any_of lists {' '.join(first)!r} twice" + raise ValueError(msg) + # Sorting by length is total here: two DISTINCT entries of equal length + # cannot prefix each other, since an equal-length prefix is the same list. + shorter, longer = sorted((first, second), key=len) + if longer[: len(shorter)] == shorter: + msg = ( + f"{label} verb_any_of entry {' '.join(shorter)!r} is a prefix of " + f"{' '.join(longer)!r}; the shorter one already accepts every invocation the " + "longer one does, so drop the longer entry or list only the verbs you mean." + ) + raise ValueError(msg) + + +def validate_positional(positional: list[str] | None, label: str) -> None: + """Reject an empty positional list, which slices to itself and asserts nothing.""" + if positional is not None and not positional: + msg = ( + f"{label} positional must not be empty: an empty list asserts nothing. List the " + "arguments you expect, or drop the field." + ) + raise ValueError(msg) + + +def validate_flag_ownership(flags: dict[str, FlagMatch] | None, ignore_flags: list[str], label: str) -> None: + """Reject flag predicates that collide with each other or with ``ignore_flags``. + + An alias that is also a key, or shared between two predicates, would make + which predicate owns a recorded flag depend on dict order. A predicate on an + ignored flag can never be evaluated: ignore_flags drops the flag before any + predicate runs, so ``absent`` would pass vacuously and ``equals`` could never + match. + """ + seen: dict[str, str] = {} + for key, predicate in (flags or {}).items(): + for name in (key, *predicate.aliases): + if name in seen and seen[name] != key: + msg = ( + f"{label} flag name {name!r} is claimed by both {seen[name]!r} and {key!r} " + "(via aliases); a flag can belong to only one predicate" + ) + raise ValueError(msg) + seen[name] = key + if key in predicate.aliases: + msg = f"{label} flag {key!r} lists itself in aliases" + raise ValueError(msg) + + shadowed = sorted(set(seen) & set(ignore_flags)) + if shadowed: + names = ", ".join(repr(n) for n in shadowed) + msg = ( + f"{label} flag predicate(s) {names} are also listed in ignore_flags (directly or as " + "an alias), which drops them before matching. Remove them from ignore_flags, or drop " + "the predicate." + ) + raise ValueError(msg) + + +def build_match_spec( + *, + verb_spellings: list[list[str]], + positional: list[str] | None, + flags: dict[str, FlagMatch] | None, + value_flags: list[str], + ignore_flags: list[str], +) -> MatchSpec: + """Lower an authored match surface to what :mod:`coder_eval.argv_match` reads. + + JSON-serializable on purpose: the same dict is embedded verbatim into a + generated shim, so a spec the criterion evaluates in-process and a spec the + shim evaluates in the sandbox are the same bytes. + + The cast is honest because ``FlagMatch``'s field set IS ``FlagPredicate``'s key + set -- asserted in tests/test_cli_match_parity.py, so adding a field to one and + not the other fails rather than silently dropping out of the lowered spec. + """ + return { + "verb_spellings": verb_spellings, + "positional": positional, + "flags": ( + {name: cast(FlagPredicate, predicate.model_dump()) for name, predicate in flags.items()} if flags else None + ), + "value_flags": list(value_flags), + "ignore_flags": list(ignore_flags), + } + + +class CliMatch(BaseModel): + """A pattern over ONE invocation's arguments, used to dispatch a canned response. + + The ``when:`` block of a ``record_cli`` response rule. Facets are ANDed, and + an unmentioned facet is unconstrained — an extra ``--output json`` never + stops a rule from matching. Matching semantics are identical to the + ``cli_called`` criterion of the same shape, so the pattern that selects a + stub response is the pattern that grades it. + + Always a mapping, never a bare string: a pattern has six possible facets, so + a lone ``"ixp dummy1"`` would leave the reader to infer which one it sets, and + a quoted verb reads enough like a command line to invite the flags a verb + cannot hold. :class:`FlagMatch` one level down keeps its scalar shorthand + (``flags: {output: json}``) — a single-valued predicate has only one facet a + scalar could mean, so nothing is left to infer there. + """ + + model_config = ConfigDict(extra="forbid") + + verb: str | None = Field( + default=None, + description=( + "Whitespace-separated subcommand chain that must be an ORDERED PREFIX of the " + "invocation's non-flag arguments, compared token by token (so 'projects list' never " + "matches 'projects lists', and 'labellings confirm' never matches 'labellings " + "unconfirm'). Tokens after it are unconstrained, so a short verb claims every " + "invocation under it: 'projects' answers 'projects delete' as readily as 'projects get'" + ), + ) + verb_any_of: list[str] | None = Field( + default=None, + description=( + "Alternative whole verbs; matches if ANY of them does, e.g. ['projects list', " + "'projects get'] to serve one response for both spellings. Each entry is a complete " + "verb in the same form `verb` takes, NOT one token of a chain. Mutually exclusive with " + "`verb`" + ), + ) + positional: list[str] | None = Field( + default=None, + description=( + "Non-flag arguments that must follow the verb, in order. A PREFIX of what followed, so " + "anything past them is unconstrained. Use it to answer differently per project/id, e.g. " + "positional: ['proj-1']. Depends on value_flags being complete — an undeclared flag's " + "value stays non-flag and shifts these slots" + ), + ) + flags: dict[str, FlagMatch] | None = Field( + default=None, + description=( + "Flag name (without leading dashes) to predicate. A bare scalar means 'equals', e.g. " + "flags: {output: json} to serve JSON only when the agent asked for it. Flags not listed " + "are ignored, so an unrelated flag never stops the rule matching" + ), + ) + value_flags: list[str] = Field( + default_factory=lambda: ["output"], + description=( + "Flag names (no leading dashes) that consume a following token as their value. Keys of " + "`flags` are value-bearing already; everything else is a switch whose following token " + "stays positional. Declare a flag here when its value would otherwise be read as a " + "positional, e.g. [folder] for `--folder F proj-1`. Defaults to [output]" + ), + ) + ignore_flags: list[str] = Field( + default_factory=list, + description=( + "Flag names dropped before matching. Empty by default, unlike the cli_called criterion: " + "a response rule dispatches rather than grades, so nothing is outcome-invisible here and " + "a rule may key on any flag it declares" + ), + ) + + @property + def verb_spellings(self) -> list[list[str]]: + """Each accepted verb as its token list; empty when there is no verb constraint.""" + return verb_spellings_of(self.verb, self.verb_any_of) + + @property + def match_spec(self) -> MatchSpec: + """This pattern as what :func:`coder_eval.argv_match.argv_matches` reads.""" + return build_match_spec( + verb_spellings=self.verb_spellings, + positional=self.positional, + flags=self.flags, + value_flags=self.value_flags, + ignore_flags=self.ignore_flags, + ) + + @model_validator(mode="before") + @classmethod + def _reject_scalar_shorthand(cls, value: Any) -> Any: + """Name the fix, rather than let pydantic report a bare type error. + + ``when: "ixp dummy1"`` is the obvious thing to try, and the generic + "Input should be a valid dictionary" says nothing about which key was + meant. + """ + if isinstance(value, str): + msg = f'record_cli response `when` must be a mapping, not a bare string: use {{verb: "{value}"}}' + raise ValueError(msg) + return value + + @model_validator(mode="after") + def _validate_match(self) -> CliMatch: + label = "record_cli response `when`" + validate_verbs(self.verb, self.verb_any_of, self.verb_spellings, label) + validate_positional(self.positional, label) + validate_flag_ownership(self.flags, self.ignore_flags, label) + # Falsiness, not `is None`: `verb: ""` would otherwise match every + # invocation and shadow every rule below it. + if not self.verb and not self.verb_any_of and not self.positional and not self.flags: + msg = ( + "record_cli response `when` requires at least one of verb / verb_any_of / positional " + "/ flags. A rule that matches everything is the tool's default response: set the " + "entry's own exit_code / stdout / stderr instead." + ) + raise ValueError(msg) + return self diff --git a/src/coder_eval/models/criteria.py b/src/coder_eval/models/criteria.py index b0822638..fe74662f 100644 --- a/src/coder_eval/models/criteria.py +++ b/src/coder_eval/models/criteria.py @@ -8,14 +8,22 @@ from __future__ import annotations -import itertools from abc import ABC, abstractmethod from pathlib import PurePosixPath from typing import Annotated, Any, ClassVar, Literal, Self from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator +from coder_eval.argv_match import MatchSpec from coder_eval.models.agent_config import AgentConfig, ClaudeCodeAgentConfig, parse_agent_config +from coder_eval.models.cli_match import ( + FlagMatch, + build_match_spec, + validate_flag_ownership, + validate_positional, + validate_verbs, + verb_spellings_of, +) from coder_eval.models.enums import AgentKind from coder_eval.models.judge_defaults import DEFAULT_JUDGE_MODEL from coder_eval.models.sandbox import RECORD_CLI_LOG @@ -420,112 +428,6 @@ class FileMatchesRegexCriterion(BaseSuccessCriterion): flags: int = Field(default=0, description="Regex flags (e.g., re.IGNORECASE=2, re.MULTILINE=8, re.DOTALL=16)") -class FlagMatch(BaseModel): - """Predicate for ONE flag value within :class:`CliCalledCriterion`. - - Exactly one predicate field may be set. In YAML a bare scalar is accepted as - shorthand for ``equals`` (``model: gemini_2_5_pro`` == ``model: {equals: - gemini_2_5_pro}``), which keeps the common case unnested. - - ``absent: true`` asserts the flag was NOT passed — distinct from "passed with - a different value", and the reason this is a predicate rather than a bare - ``dict[str, str]`` on the criterion. - - The one-predicate rule means a conjunction on a single flag ("contains BOTH - A and B") is not expressible here. Either declare two ``cli_called`` criteria - over the same log, or use one ``matches_regex`` that spans both — the latter - is what a heredoc-built JSON payload usually wants, together with - ``flags: 16`` (``re.DOTALL``) so ``.`` crosses the payload's newlines. - """ - - model_config = ConfigDict(extra="forbid") - - equals: str | None = Field(default=None, description="Flag value must equal this string exactly") - contains: str | None = Field(default=None, description="Flag value must contain this substring") - matches_regex: str | None = Field( - default=None, - description="Flag value must match this regex. Scoped to ONE value, unlike a whole-line pattern", - ) - any_of: list[str] | None = Field( - default=None, - min_length=1, - description=( - "Flag value must equal one of these strings. Non-empty: an empty list would match " - "nothing, so a max_count: 0 guard built on it would pass vacuously" - ), - ) - absent: bool = Field(default=False, description="Flag must NOT be present in the invocation") - present: bool = Field( - default=False, - description=( - "Flag must be present, whatever its value -- the predicate for a boolean switch. Unlike " - '`equals: ""` it survives a CLI that spells the switch `--force true`, and it never makes ' - "the flag value-bearing, so asserting a switch cannot swallow the next positional" - ), - ) - aliases: list[str] = Field( - default_factory=list, - description=( - "Other names for the SAME flag, e.g. aliases: [y] on a `yes` predicate so `-y` and " - "`--yes` are one flag. Values are gathered across every name: `present` holds if any " - "appeared, `absent` only if none did, a value predicate matches if any value under any " - "name satisfies it" - ), - ) - flags: int = Field( - default=0, - description=( - "Regex flags for matches_regex (re.IGNORECASE=2, re.MULTILINE=8, re.DOTALL=16), " - "mirroring FileMatchesRegexCriterion.flags. DOTALL is the usual need, since a " - "heredoc-built flag value spans lines" - ), - ) - - @property - def needs_value(self) -> bool: - """Whether evaluating this predicate requires the flag's VALUE. - - Presence predicates (``present`` / ``absent``) do not, so they must not - make a flag value-bearing. Otherwise asserting a boolean switch would - make it consume the following token: adding ``flags: {yes: {present: - true}}`` to a guard on ``delete --yes proj-1`` would bind - ``yes=proj-1``, drop ``proj-1`` from the positionals, and hand the guard - a false PASS -- reintroducing the very defect declared value-binding - exists to prevent. - """ - return not (self.present or self.absent) - - @model_validator(mode="before") - @classmethod - def _coerce_scalar_shorthand(cls, value: Any) -> Any: - """Accept ``model: gemini_2_5_pro`` as ``model: {equals: ...}``.""" - if isinstance(value, str): - return {"equals": value} - return value - - @model_validator(mode="after") - def _exactly_one_predicate(self) -> FlagMatch: - set_predicates = [ - name for name in ("equals", "contains", "matches_regex", "any_of") if getattr(self, name) is not None - ] - if self.absent: - set_predicates.append("absent") - if self.present: - set_predicates.append("present") - if len(set_predicates) != 1: - msg = ( - "FlagMatch requires exactly one of equals / contains / matches_regex / any_of / absent / present, " - f"got {sorted(set_predicates) or 'none'}" - ) - raise ValueError(msg) - # `flags` only reaches re.compile via matches_regex; setting it beside any - # other predicate is a silent no-op, so reject it rather than mislead. - 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}" - raise ValueError(msg) - return self - - class CliCalledCriterion(BaseSuccessCriterion): """Check whether a CLI invocation matching a structured pattern was recorded. @@ -543,11 +445,29 @@ class CliCalledCriterion(BaseSuccessCriterion): Only ``argv`` is required. ``tool`` enables one log to serve several shadowed executables; ``exit`` and ``ts`` are recorded for reporting, not matched. + A generated ``record_cli`` shim adds ``rule`` (the index of the response rule + that answered), which is reporting only, plus two keys that are NOT ignored + because each means the responses the agent saw were not the ones the task + described: ``sidecar_error`` (the shim could not import its matcher module) + and ``rule_error`` (rule evaluation raised). Both fail the criterion. Neither + escalates, because the recorder directory sits inside the sandbox the agent + writes to, so both are agent-reachable; an authoring mistake is caught + earlier instead, by :class:`~coder_eval.models.RecordedCli`'s load-time + check that every response rule is evaluable. + Why not ``file_matches_regex`` over a flattened log line: a flat line cannot express "verb X was called AND flag Y had value Z" without stacked lookaheads, cannot tell a quoted argument containing spaces from two arguments, and cannot stop a match from running across shell operators. + EVIDENCE, NOT ATTESTATION. The log is an ordinary file in the sandbox the + agent writes to, so an agent that wants to can append a record for a call it + never made, or delete one it did. This criterion is built to keep an HONEST + run honest -- a missing or unreadable log fails rather than passing a + ``max_count: 0`` guard vacuously -- not to withstand an adversary. Do not + build an anti-cheat control on it; see ``tasks/anti_cheat_reference`` and + docs/DOCKER_ISOLATION.md for what that requires. + Pure data model - checking logic in CliCalledChecker._check_impl() Example YAML (positive — flag value must match):: @@ -664,40 +584,28 @@ def verb_spellings(self) -> list[list[str]]: The only place either verb field is split, so the validators, the matcher and the failure detail cannot disagree. """ - if self.verb is not None: - return [self.verb.split()] - if self.verb_any_of is not None: - return [spelling.split() for spelling in self.verb_any_of] - return [] + return verb_spellings_of(self.verb, self.verb_any_of) + + @property + def match_spec(self) -> MatchSpec: + """This criterion's argv facets as what :mod:`coder_eval.argv_match` reads. + + The same lowering a ``record_cli`` response rule uses, so a rule that + serves a response and the criterion that grades it cannot read one argv + two ways. ``tool`` stays out: it matches a log record's field, not argv. + """ + return build_match_spec( + verb_spellings=self.verb_spellings, + positional=self.positional, + flags=self.flags, + value_flags=self.value_flags, + ignore_flags=self.ignore_flags, + ) @model_validator(mode="after") def _validate_verb(self) -> CliCalledCriterion: """Verb rules, kept off _validate_bounds so neither grows unreadable.""" - if self.verb is not None and self.verb_any_of is not None: - msg = "cli_called accepts verb or verb_any_of, not both" - raise ValueError(msg) - # Falsy, so the at-least-one-facet check below would read it as "no verb". - if self.verb_any_of is not None and not self.verb_any_of: - msg = "cli_called verb_any_of must not be empty: drop the field to match any verb" - raise ValueError(msg) - # A character count would pass " ", whose split() is an empty prefix. - if any(not tokens for tokens in self.verb_spellings): - msg = "cli_called verb must not be blank: a blank verb is an empty prefix and matches every record" - raise ValueError(msg) - for first, second in itertools.combinations(self.verb_spellings, 2): - if first == second: - msg = f"cli_called verb_any_of lists {' '.join(first)!r} twice" - raise ValueError(msg) - # Sorting by length is total here: two DISTINCT entries of equal length - # cannot prefix each other, since an equal-length prefix is the same list. - shorter, longer = sorted((first, second), key=len) - if longer[: len(shorter)] == shorter: - msg = ( - f"cli_called verb_any_of entry {' '.join(shorter)!r} is a prefix of " - f"{' '.join(longer)!r}; the shorter one already accepts every invocation the " - "longer one does, so drop the longer entry or list only the verbs you mean." - ) - raise ValueError(msg) + validate_verbs(self.verb, self.verb_any_of, self.verb_spellings, "cli_called") return self @model_validator(mode="after") @@ -715,45 +623,15 @@ def _validate_bounds(self) -> CliCalledCriterion: raise ValueError(msg) # Matching slices an empty expectation and compares it to itself, so this reads # as "took no arguments" while asserting nothing. - if self.positional is not None and not self.positional: - msg = ( - "cli_called positional must not be empty: an empty list asserts nothing. List the " - "arguments you expect, or drop the field." - ) - raise ValueError(msg) + validate_positional(self.positional, "cli_called") # Falsiness, not `is None`: `verb: ""` slipped past an `is None` check here and - # then matched every record, scoring 1.0. + # then matched every record, scoring 1.0. `tool` counts as a facet here (but not + # for a response rule), since a criterion may legitimately count every + # invocation of one shadowed executable. if not self.verb and not self.verb_any_of and not self.positional and not self.flags and not self.tool: msg = "cli_called requires at least one of verb / verb_any_of / positional / flags / tool to match on" raise ValueError(msg) - # A predicate on an ignored flag can never be evaluated: ignore_flags drops - # the flag before any predicate runs, so `absent` would pass vacuously and - # `equals` could never match. - # An alias that is also a key, or shared between two predicates, would make - # which predicate owns a recorded flag depend on dict order. - seen: dict[str, str] = {} - for key, predicate in (self.flags or {}).items(): - for name in (key, *predicate.aliases): - if name in seen and seen[name] != key: - msg = ( - f"cli_called flag name {name!r} is claimed by both {seen[name]!r} and {key!r} " - "(via aliases); a flag can belong to only one predicate" - ) - raise ValueError(msg) - seen[name] = key - if key in predicate.aliases: - msg = f"cli_called flag {key!r} lists itself in aliases" - raise ValueError(msg) - - shadowed = sorted(set(seen) & set(self.ignore_flags)) - if shadowed: - names = ", ".join(repr(n) for n in shadowed) - msg = ( - f"cli_called flag predicate(s) {names} are also listed in ignore_flags (directly or as " - "an alias), which drops them before matching. Remove them from ignore_flags, or drop " - "the predicate." - ) - raise ValueError(msg) + validate_flag_ownership(self.flags, self.ignore_flags, "cli_called") return self diff --git a/src/coder_eval/models/sandbox.py b/src/coder_eval/models/sandbox.py index d2083d09..7b3171f6 100644 --- a/src/coder_eval/models/sandbox.py +++ b/src/coder_eval/models/sandbox.py @@ -7,6 +7,7 @@ from pydantic import AliasChoices, BaseModel, ConfigDict, Field, field_validator, model_validator +from coder_eval.models.cli_match import CliMatch from coder_eval.models.container_paths import CONTAINER_WORK_DIR, RESERVED_CONTAINER_DIRS from coder_eval.models.merge_strategy import MergeField from coder_eval.models.templates import TemplateSource @@ -315,6 +316,11 @@ def _validate_working_dir(cls, v: str | None) -> str | None: RECORD_CLI_LOG_NAME = "calls.jsonl" RECORD_CLI_LOG = f"{RECORD_CLI_DIR}/{RECORD_CLI_LOG_NAME}" +# Modules copied into the recorder directory beside each shim that declares +# response rules. The shim imports them as siblings, so they must be +# stdlib-only (lint rule CE048) -- they run where coder_eval is not installed. +SIDECAR_MODULES: tuple[str, ...] = ("argv_match.py",) + # Shadowing any of these breaks the harness rather than the tool under test: the # shim is a script run by an interpreter, and its directory goes FIRST on a PATH # the orchestrator also reuses for run_command criteria. `tool: python3` made the @@ -325,6 +331,40 @@ def _validate_working_dir(cls, v: str | None) -> str | None: ) +class CliResponse(BaseModel): + """One canned response, served when an invocation matches ``when``. + + The reason a shadowed tool can answer `uip ixp dummy1` and `uip ixp dummy2` + differently instead of returning one fixed pair of streams for everything an + agent types. Rules are tried in declaration order and the FIRST match wins, + so the specific rule goes above the general one; an invocation matching no + rule falls back to the entry's own ``exit_code`` / ``stdout`` / ``stderr``. + + ``exit_code`` defaults to 0 here, the opposite of :class:`RecordedCli`: a rule + exists because the author described this exact invocation, so the natural + reading is "and this is what it answers", whereas an undescribed one should + look like a tool that failed rather than a silent success. + """ + + model_config = ConfigDict(extra="forbid") + + when: CliMatch = Field( + description=( + 'Pattern the invocation must match, e.g. {verb: "ixp dummy1"}. Always a mapping -- same ' + "facets and same matching semantics as the cli_called criterion, so the pattern that " + "serves a response is the pattern that grades it" + ) + ) + exit_code: int = Field( + default=0, + ge=0, + le=255, + description="Exit status the shim returns for a matching invocation. Defaults to 0 (success)", + ) + stdout: str = Field(default="", description="Text the shim writes to stdout for a matching invocation") + stderr: str = Field(default="", description="Text the shim writes to stderr for a matching invocation") + + class RecordedCli(BaseModel): """One executable to shadow with a generated recording shim. @@ -335,6 +375,11 @@ class RecordedCli(BaseModel): ran without hand-rolling a mock and without the record shape being a contract between two repositories. + The fields below are what every invocation gets; ``responses`` overrides them + per invocation, so one shadowed ``uip`` can answer ``ixp dummy1`` and + ``ixp dummy2`` differently — what an agent needs when its next step depends on + what the tool just told it. + It stubs a tool; it does not proxy one. A test that needs a REAL executable's behavior recorded on the way through still supplies its own wrapper under ``mock_path_dirs`` — that depends on the tool being installed, on PATH order, @@ -369,6 +414,115 @@ class RecordedCli(BaseModel): "would, so an agent reads a plausible error rather than silence" ), ) + # Plain Field, not MergeField: `RecordedCli` is never a merge root. The + # enclosing `SandboxConfig.record_cli` is a `replace` list, so a later layer + # substitutes the whole list of entries and no per-entry strategy is ever + # consulted. A strategy annotation here would read as a knob and be inert. + responses: list[CliResponse] = Field( + default_factory=list, + description=( + "Per-invocation responses, tried in order until one matches; the fields above are the " + "fallback for an invocation none of them claim. Use it when the agent's next step " + "depends on what the tool answered -- `ixp projects list` returning a project the agent " + "then acts on, say -- instead of one fixed reply to everything. A config layer that sets " + "record_cli replaces the whole list of entries, this one included" + ), + ) + + @model_validator(mode="after") + def _validate_responses_are_reachable(self) -> RecordedCli: + """Reject a rule an earlier rule already claims. + + First-match-wins means a rule below a more general one can never answer. + Silence there would be out of step with the rest of this authoring + surface, which hard-errors on every declaration that cannot take effect: + a `verb_any_of` entry prefixed by another, a predicate on an ignored + flag, an empty `positional`, two entries writing the same shim filename. + + Deliberately narrow, because "A matches everything B matches" is not + decidable in general. Two sound cases only: an exact duplicate, and a + verb-only A whose verb prefixes B's under the same flag parsing. + """ + specs = [response.when.match_spec for response in self.responses] + for later, spec in enumerate(specs): + for earlier, prior in enumerate(specs[:later]): + if prior == spec: + reason = "is an exact duplicate of" + elif ( + prior["positional"] is None + and prior["flags"] is None + # BOTH sides free of flag predicates, not just the earlier + # one: a predicate makes its flag known and value-bearing in + # that rule's parse only. `--profile prod ixp projects get` + # leaves `prod` positional for a verb-only `ixp projects`, + # which therefore does NOT match, while a later + # `ixp projects get` + `flags: {profile: prod}` does -- so the + # later rule is reachable and rejecting it was wrong. + and spec["flags"] is None + and prior["value_flags"] == spec["value_flags"] + and prior["ignore_flags"] == spec["ignore_flags"] + and spec["verb_spellings"] + and all( + any(tokens[: len(prefix)] == prefix for prefix in prior["verb_spellings"]) + for tokens in spec["verb_spellings"] + ) + ): + reason = "is already claimed by the more general" + else: + continue + msg = ( + f"record_cli tool {self.tool!r}: responses[{later}] {reason} responses[{earlier}], " + "so it can never answer -- the first matching rule wins. Put the specific rule " + "above the general one, or drop the duplicate." + ) + raise ValueError(msg) + return self + + @model_validator(mode="after") + def _validate_responses_are_evaluable(self) -> RecordedCli: + """Prove every rule can actually be MATCHED, not merely parsed. + + The shim catches a matcher fault so a broken rule cannot turn the stub into + a crashing executable, and books ``rule_error`` on the record. But + ``cli_called`` can only score that 0.0 -- the log lives in the sandbox the + agent writes to, so a fault there cannot be attributed to the task author + and must not escalate (see the comment in ``criteria/cli_called.py``). + + So the attribution has to happen HERE, before a sandbox exists and where + nothing the agent does can participate: run the real matcher over each rule + and let an unevaluable spec be a load-time ValidationError. Exercises both + branches -- an argv rebuilt from the rule's own pattern (so the rule + matches) and an empty argv (so it does not) -- because a predicate can raise + on one path and not the other. + """ + from coder_eval.argv_match import select_rule + + if not self.responses: + return self + # Building the probe argvs reads the same spec the matcher will, so it sits + # INSIDE the guard: a spec malformed enough to break this loop is exactly + # the kind that must surface as a clean authoring error, not a TypeError + # escaping a validator. + try: + rules = [ + {"when": response.when.match_spec, "exit": response.exit_code, "stdout": "", "stderr": ""} + for response in self.responses + ] + probes: list[list[str]] = [[]] + for spec in (rule["when"] for rule in rules): + for spelling in spec["verb_spellings"] or [[]]: + probes.append([*spelling, *(spec["positional"] or [])]) + for argv in probes: + select_rule(rules, argv) # type: ignore[arg-type] + except Exception as exc: + msg = ( + f"record_cli tool {self.tool!r}: a response rule cannot be evaluated " + f"({type(exc).__name__}: {exc}). The generated shim would swallow this and serve " + "the entry fallback for every invocation, so the agent would never see the " + "responses this task describes. Fix the `when:` pattern." + ) + raise ValueError(msg) from exc + return self @field_validator("tool") @classmethod @@ -391,8 +545,23 @@ def validate_tool_name(cls, v: str) -> str: + f"Reserved: {reserved}" ) raise ValueError(msg) - if v == RECORD_CLI_LOG_NAME: + # Folded for the same reason as the reserved set: on a case-insensitive + # filesystem `CALLS.JSONL` is the seeded log, and the shim write would hit + # it -- reported as a confusing duplicate-filename error at setup instead. + if v.lower() == RECORD_CLI_LOG_NAME: raise ValueError(f"record_cli tool {v!r} would overwrite the invocation log criteria read") + # Case-folded like the reserved check above: APFS and NTFS are + # case-insensitive, so `ARGV_MATCH.PY` names the same inode as the + # sidecar. The sidecar write would then clobber the agent's shim without + # `_generate_cli_recorders`' per-tool exists() guard ever firing. + if v.lower() in {module.lower() for module in SIDECAR_MODULES}: + names = ", ".join(sorted(SIDECAR_MODULES)) + msg = ( + f"record_cli tool {v!r} collides with a module the recorder writes beside the shim " + f"({names}); the shim imports it as a sibling, so shadowing it breaks response " + "dispatch for every entry. Declare a different name." + ) + raise ValueError(msg) if v.lower().endswith((".cmd", ".bat")): raise ValueError(f"record_cli tool {v!r} collides with the generated Windows twin; declare the bare name") return v @@ -458,10 +627,11 @@ class SandboxConfig(BaseModel): "Executables to shadow with a generated recording shim. The sandbox writes each shim " f"into '{RECORD_CLI_DIR}/' and PATH-prepends that directory, so the agent's calls are " f"recorded as JSON Lines in '{RECORD_CLI_LOG}' — the log a 'cli_called' criterion reads " - "by default. Use instead of hand-writing a mock under mock_path_dirs when all the test " - "needs is a faithful record of what ran plus a canned exit status and message. It does " - "NOT serve per-invocation responses and does NOT proxy the real executable; supply your " - "own mock for either. Replaced (not merged) across config layers, like mock_path_dirs." + "by default. Use instead of hand-writing a mock under mock_path_dirs when the test needs " + "a faithful record of what ran plus canned output -- one reply per entry, or a different " + "one per invocation via that entry's 'responses'. It does NOT proxy the real executable " + "(nothing is run, so no network, auth, or side effect); supply your own mock for that. " + "Replaced (not merged) across config layers, like mock_path_dirs." ), ) diff --git a/src/coder_eval/sandbox.py b/src/coder_eval/sandbox.py index 55a24b5a..5323693e 100644 --- a/src/coder_eval/sandbox.py +++ b/src/coder_eval/sandbox.py @@ -14,10 +14,11 @@ from pathlib import Path from .fs_permissions import RESTRICTED_MODE, set_permissions -from .invocation_log import render_recorder +from .invocation_log import render_recorder, sidecar_source from .models import ( RECORD_CLI_DIR, RECORD_CLI_LOG, + SIDECAR_MODULES, RepoSource, SandboxConfig, StarterFilesSource, @@ -562,11 +563,15 @@ def resolved_mock_path_dirs(self) -> list[Path]: def _generate_cli_recorders(self) -> None: """Write a recording shim for every ``SandboxConfig.record_cli`` entry. - Each shim is a self-contained Python script — it must run inside the - sandbox, where ``coder_eval`` is not installed, so it imports nothing - from this package and carries its configuration as embedded literals. - A ``.cmd`` twin is written beside it so a bare ``uip`` also resolves - through Windows PATHEXT lookup on the tempdir driver. + Each shim runs inside the sandbox, where ``coder_eval`` is not + installed, so it carries its configuration as literals and imports + nothing installed. An entry that declares ``responses`` also gets every + :data:`SIDECAR_MODULES` file written into the recorder directory beside + it — the argv matcher it dispatches on, which it imports as a sibling + rather than the harness splicing that source into the shim. A rules-less + entry needs no matcher, so no sidecar is written for it. + A ``.cmd`` twin is written beside each shim so a bare ``uip`` also + resolves through Windows PATHEXT lookup on the tempdir driver. Raises: RuntimeError: a task's own ``mock_path_dirs`` already provides an @@ -645,10 +650,22 @@ def _generate_cli_recorders(self) -> None: newline="", ) - logger.info( - f"Generated {len(self.config.record_cli)} CLI recorder(s) in {RECORD_CLI_DIR}/: " - + ", ".join(f"{s.tool}(exit {s.exit_code})" for s in self.config.record_cli) + # Once for the whole directory, not once per entry: every rules-bearing shim + # imports the same sidecar, so writing it inside the loop above just rewrote + # identical bytes N times. Skipped entirely when no entry declares rules -- + # such a shim never consults the matcher and needs no sibling file. + sidecars = sorted(SIDECAR_MODULES) if any(spec.responses for spec in self.config.record_cli) else [] + for module in sidecars: + (recorder_dir / module).write_text(sidecar_source(module), encoding="utf-8", newline="\n") + + summary = ", ".join( + f"{s.tool}(exit {s.exit_code}" + (f", {len(s.responses)} rule(s)" if s.responses else "") + ")" + for s in self.config.record_cli ) + # Names the sidecar: an operator debugging a `sidecar_error` needs setup-time + # confirmation that the file was actually written. + beside = f" (+ {', '.join(sidecars)})" if sidecars else "" + logger.info(f"Generated {len(self.config.record_cli)} CLI recorder(s) in {RECORD_CLI_DIR}/{beside}: {summary}") def _apply_starter_files_source(self, source: StarterFilesSource) -> None: """Create inline starter files in sandbox with overwrite tracking. diff --git a/tasks/README.md b/tasks/README.md index 6539c58f..dcc8f36a 100644 --- a/tasks/README.md +++ b/tasks/README.md @@ -27,6 +27,7 @@ docs. | `inline_starter_example` | inline starter files | | `sentiment_classification` | `classification_match` + a JSONL dataset (`datasets/`) | | `mock_path_dirs_smoke` | mocking CLIs on `PATH` (uses `mock_path_dirs_template_dir/`) | +| `record_cli_responses` | `record_cli` per-invocation `responses` + `cli_called` grading (`driver: docker`) | | `test_sandbox` | the smallest possible sandbox task | ## `agents/` — agent feature-tests @@ -52,9 +53,10 @@ would drop them from those runs. | `smoke` | Umbrella over the pass + fail buckets | ad hoc | Members: `hello_date`, `agentless_smoke_test`, `byod_smoke_test`, -`dataset_example`, `smoke_agent_judge`, `smoke_llm_judge`, `smoke_negative_path`, -`smoke_budget_exceeded`, `smoke_cost_budget_exceeded`, `smoke_task_timeout`, -`smoke_variants`, `token_check`. +`dataset_example`, `opencode_smoke_test`, `record_cli_responses`, `smoke_agent_judge`, +`smoke_llm_judge`, `smoke_negative_path`, `smoke_budget_exceeded`, +`smoke_cost_budget_exceeded`, `smoke_task_timeout`, `smoke_variants`, +`token_check`. ## Support subdirectories diff --git a/tasks/record_cli_responses.yaml b/tasks/record_cli_responses.yaml new file mode 100644 index 00000000..b3c0943d --- /dev/null +++ b/tasks/record_cli_responses.yaml @@ -0,0 +1,206 @@ +task_id: record_cli_responses +description: | + Probe: does a generated `record_cli` shim really serve a different canned + response per invocation, inside a container, and does the agent see it? + + Everything below is covered by unit tests EXCEPT the one thing this task + exists for: the shim is generated by the in-container orchestrator, so its + shebang interpreter is `os.path.realpath(sys.executable)` as resolved INSIDE + the container. `run_task_internal_command` rewrites `driver: docker` -> + `tempdir` before building that orchestrator, so nothing running on the host + can exercise that path. Hence `driver: docker`. + + What it exercises end to end: + + 1. Shim generation inside the container, with the container's python baked + into the shebang. + 2. The SIDECAR import: `argv_match.py` is written into `cli_mocks/` beside + the shim and imported as a sibling. If that import fails, the shim serves + the entry fallback for everything and books `sidecar_error` on every + record; the two `cli_called` criteria then fail with the precise + diagnostic ("could not import its matcher module"), which is the one to + read first. + 3. Per-invocation rule dispatch: `ixp dummy1` and `ixp dummy2` must answer + DIFFERENTLY, and `ixp nope` must fall through to the entry default. + 4. `cli_called` grading the resulting JSON Lines log. + + Why three criterion families and not just `cli_called`: the two `cli_called` + criteria prove the invocations were RECORDED. They cannot tell per-rule + dispatch from every rule serving the entry default -- both look identical in + the log's argv. + + The two regex checks on cli_mocks/calls.jsonl are the AUTHORITATIVE dispatch + detectors: the shim writes `"rule": N` only when rule N actually answered, so + no prompt-compliance failure can suppress it, and unlike the captured text it + cannot be satisfied by reading this YAML. Each pattern ties one argv to ITS OWN + rule index, because requiring `"rule": 0` and `"rule": 1` to appear merely + somewhere would accept a regression that swapped them -- dispatch happening, + but on the wrong invocations. + + What they are NOT is tamper-proof. The log lives in the sandbox the agent + writes to, so a determined agent could append a matching line -- the same + property that lets any `cli_called` criterion be satisfied by a hand-written + record. This probe is a REGRESSION detector against coder_eval's own codegen, + aimed at a cooperative agent; it is not an anti-cheat control. The adversarial + probe is tasks/anti_cheat_reference. + + The `captured.txt` check is the weaker, end-user-visible half: it shows the + response text reached the AGENT, not just the log. It is deliberately NOT + treated as dispatch proof, because this YAML is serialised to /work/input + (readable) and mounted again at /work/task_dir, so RESPONSE_ONE / RESPONSE_TWO + are reachable with `cat` -- an agent that found empty stdout and went looking + could transcribe them. Keeping them out of the prompt (pinned by + tests/test_tags.py::TestRecordCliProbeIntegrity) narrows that path but cannot + close it; the log criterion is what closes it. + + Run it with: + coder-eval run tasks/record_cli_responses.yaml + + PREREQUISITE: `make docker-image`. This is a `driver: docker` task; without + the image it fails at sandbox setup. + + CI: tagged smoke-pass, so it lands in the e2e-smoke "expect all to succeed" + bucket and is counted by EXPECTED_SMOKE_PASS_RUN / _SUCCEEDED in + .github/workflows/pr-checks.yml. That bucket is BLOCKING. + + If this task ever proves flaky, the lever is to drop every AGENT-DEPENDENT + criterion to `weight: 0.0` -- the two `cli_called` and both `captured.txt` + criteria -- and let the two `cli_mocks/calls.jsonl` regexes gate alone. They need + nothing from the agent beyond having run the two commands, so what survives + still proves dispatch rather than proving nothing. `is_gating` is `weight > 0`, + so there is no "advisory but gating" middle setting. + +tags: + # smoke / smoke-pass put this in the CI e2e-smoke "expect all to succeed" + # bucket, which globs `tasks/*.yaml`. This file is FLAT on purpose so that + # glob matches it and neither the workflow step nor Makefile SMOKE_GLOBS needs + # a new path. Adding/removing it means bumping the EXPECTED_SMOKE_PASS_* pair. + - smoke + - smoke-pass + - record-cli + - docker + +agent: + type: claude-code + permission_mode: acceptEdits + # Mirrors anti_cheat_reference. Read is kept deliberately: Claude Code's Write + # tool refuses to overwrite a file the session has not read, so an agent that + # wants to correct or extend captured.txt would otherwise burn turns against + # max_turns discovering the Bash heredoc fallback. Restricting tools would NOT + # make the response strings unreachable anyway -- see the description block. + allowed_tools: + - Bash + - Read + - Write + # No MCP servers needed; keeps the host project's CLAUDE.md and settings out + # of the sandbox (and out of every API call). + setting_sources: [] + +sandbox: + driver: docker + docker: + # bridge, not none: the agent needs to reach its own model API. Network + # isolation is not what this task tests. + network: bridge + # No `limits:` block: ResourceLimits.timeout binds only Sandbox.run_command's + # subprocess timeout, and this task declares no run_command criterion. The + # agent's caps are run_limits below. + record_cli: + - tool: uip + # Entry fallback, deliberately distinct from both rules in exit code AND + # streams, so "the fallback answered everything" is distinguishable from a + # pass (pinned by TestRecordCliProbeIntegrity). + exit_code: 1 + stderr: "uip: unknown command\n" + responses: + - when: {verb: "ixp dummy1"} + stdout: "RESPONSE_ONE\n" + - when: {verb: "ixp dummy2"} + stdout: "RESPONSE_TWO\n" + +initial_prompt: | + This sandbox has a `uip` command on your PATH. Your job is to report exactly + what it prints. + + Run these three commands, in this order, and capture the full output of each: + + 1. `uip ixp dummy1` + 2. `uip ixp dummy2` + 3. `uip ixp nope` + + The third command is expected to FAIL with a non-zero exit status. That is a + valid, useful result, not a problem to work around: record what it printed and + move on. Do not retry it, do not try to install or repair `uip`, and do not + guess at what any of the three commands "should" print -- run them and report + what you actually saw. + + Then write a file `captured.txt` in your working directory containing the + verbatim output of all three commands, each under a heading naming the command + that produced it. + +success_criteria: + - type: cli_called + verb: "ixp dummy1" + min_count: 1 + description: "The agent invoked the first stubbed subcommand, and it was recorded." + weight: 1.0 + + - type: cli_called + verb: "ixp dummy2" + min_count: 1 + description: "The agent invoked the second stubbed subcommand, and it was recorded." + weight: 1.0 + + # THE load-bearing criterion, and the only one here independent of the agent: + # the shim writes `"rule": N` only when rule N actually answered, so this fails + # if dispatch breaks no matter what the agent does, and passes even if the + # agent names its capture file something else. Note weight does not confer + # gate priority -- `all_criteria_passed` is a strict AND over every criterion + # with weight > 0, so the 1.0-weighted ones fail the task just as hard. The + # weight only shapes the reported score. + # One per rule, CORRELATING argv with the rule index that served it. Merely + # requiring `"rule": 0` and `"rule": 1` somewhere in the log would accept a + # regression that swapped them -- dispatch happening, but wrong. + - type: file_matches_regex + path: cli_mocks/calls.jsonl + pattern: '"argv": \["ixp", "dummy1"\][^\n]*"rule": 0' + must_match: true + description: >- + The first stubbed subcommand was served by ITS OWN rule (index 0), proving + real per-invocation dispatch rather than the entry fallback -- which the + cli_called pair above cannot distinguish. + weight: 2.0 + + - type: file_matches_regex + path: cli_mocks/calls.jsonl + pattern: '"argv": \["ixp", "dummy2"\][^\n]*"rule": 1' + must_match: true + description: "The second stubbed subcommand was served by its own rule (index 1)." + weight: 2.0 + + # The agent-visible half: proves the response text reached the AGENT, not just + # the log. NOT dispatch proof on its own -- this YAML is readable at + # /work/input, so the strings are transcribable (see the description block). + - type: file_contains + path: captured.txt + includes: ["RESPONSE_ONE", "RESPONSE_TWO"] + description: >- + Both per-invocation responses reached the agent, not merely the log. + weight: 1.0 + + # Redundant with the file_contains above on the same path (that checker already + # fails a missing file), kept only as an explicit, readable statement that the + # agent must produce the capture file at all. + - type: file_exists + path: captured.txt + description: "The agent produced its capture file." + weight: 1.0 + +# No cli_called criterion on `ixp nope`: the fallback is already implied by the +# two rules above answering differently, and a positive assertion on a command a +# model may skip after two successes is a pure flake source in a blocking bucket. + +run_limits: + max_turns: 6 + task_timeout: 300 + turn_timeout: 150 diff --git a/tests/lint/doc_schema_parity.py b/tests/lint/doc_schema_parity.py index 5ded814d..80fc44a9 100644 --- a/tests/lint/doc_schema_parity.py +++ b/tests/lint/doc_schema_parity.py @@ -12,10 +12,15 @@ class impossible to reintroduce: for a small, explicit registry of user-facing * **Allowlist, not denylist.** A new field on a registered model that is neither documented nor exempted *fails* — which is the point. Adding a user-facing field now forces a doc update or a reasoned exemption in the same change. -* **Explicit registry, no recursion.** Only the four registered models are +* **Explicit registry, no recursion.** Only the six registered models are checked; nested models (``AgentConfig``, ``SandboxConfig``, criteria, …) are NOT walked. Walking them would silently expand the documentation commitment to - dozens of models nobody signed up for. + dozens of models nobody signed up for. ``CliMatch`` is deliberately absent for + that reason: its fields are documented in the ``cli_called`` reference, and + registering a third nested model under ``SandboxConfig`` would start exactly + the tree-walk this bullet exists to prevent. A new field on a registered model + fails ``make lint`` until it is documented or exempted -- that is the intent, + not a bug in the rule. * **Inline-code match, deliberately simple.** A field counts as documented when its bare name appears wrapped in Markdown inline-code backticks anywhere in the doc. This is a floor, not a proof — a field name that appears in an unrelated @@ -24,6 +29,12 @@ class impossible to reintroduce: for a small, explicit registry of user-facing catch *entirely undocumented* fields, and a fuzzier "documented in the right section" rule invites false passes that erode trust in the gate. + A corollary for models that share a vocabulary: ``RecordedCli`` and + ``CliResponse`` both declare ``exit_code`` / ``stdout`` / ``stderr``, so + registering the second only newly guards ``when``, and a FUTURE field on either + that reuses a name the other already documents passes without its own doc line. + Still net-positive, but do not read a green gate here as per-model coverage. + Like CE027/CE029, this is intentionally NOT a ``BaseRule`` registered in ``tests/lint/runner.py`` (that runner is AST-only over ``.py`` files); it reasons over Markdown and is wired as ``tests/test_custom_lint.py::TestCE030DocSchemaParity``. @@ -35,7 +46,7 @@ class impossible to reintroduce: for a small, explicit registry of user-facing from pydantic import BaseModel -from coder_eval.models import Dataset, RunLimits, SimulationConfig, TaskDefinition +from coder_eval.models import CliResponse, Dataset, RecordedCli, RunLimits, SimulationConfig, TaskDefinition # Models the project commits to documenting, paired with the doc page that owns @@ -46,6 +57,8 @@ class impossible to reintroduce: for a small, explicit registry of user-facing (RunLimits, "docs/TASK_DEFINITION_GUIDE.md"), (Dataset, "docs/TASK_DEFINITION_GUIDE.md"), (SimulationConfig, "docs/TASK_DEFINITION_GUIDE.md"), + (RecordedCli, "docs/TASK_DEFINITION_GUIDE.md"), + (CliResponse, "docs/TASK_DEFINITION_GUIDE.md"), ] # Fields deliberately absent from the user docs, with the reason each is not diff --git a/tests/lint/rules/ce048_sidecar_shim_stdlib_only.py b/tests/lint/rules/ce048_sidecar_shim_stdlib_only.py new file mode 100644 index 00000000..70b8572e --- /dev/null +++ b/tests/lint/rules/ce048_sidecar_shim_stdlib_only.py @@ -0,0 +1,82 @@ +"""CE048: a sidecar module copied beside a generated sandbox shim stays stdlib-only. + +``Sandbox._generate_cli_recorders`` writes every module in +``models.sandbox.SIDECAR_MODULES`` into the recorder directory beside each +``record_cli`` shim that declares response rules, and the shim imports it as a +sibling. That sidecar runs inside the sandbox, where ``coder_eval`` is not +installed and no project dependency is guaranteed, so one +``from coder_eval.models import ...`` or ``import pydantic`` makes every shadowed +CLI die with an ImportError the moment the agent runs it. It surfaces as "the +tool is broken", never as "the harness wrote an unimportable sidecar", and it +costs a whole run to diagnose. + +Import-time enforcement (a test that renders and executes a shim) only catches it +when a test happens to declare a response rule; this rule catches it the moment +the import is written. + +A stdlib module that is genuinely needed is added to ``STDLIB_ALLOWED`` below -- +deliberately an allowlist rather than a check against ``sys.stdlib_module_names``, +so growing the sidecar's surface is a decision someone makes on purpose. + +``from __future__ import ...`` falls out of that allowlist too, and is reported +separately: it is not an import hazard (every interpreter that can run the shim +supports it), so the fix is to drop the line rather than widen the allowlist -- +which is what the generic message would otherwise suggest. +""" + +import ast +import re + +from coder_eval.models import SIDECAR_MODULES +from tests.lint.rules.base import BaseRule + + +class SidecarShimStdlibOnly(BaseRule): + id = "CE048" + + # Derived from the writer's own list, so moving the module moves the rule + # with it. A hardcoded second copy would match nothing after such a move and + # pass vacuously -- guarding zero files while reading as a guarantee. + # tests/test_custom_lint.py asserts the pattern matches a file that exists. + _SIDECAR = re.compile(r"[/\\]coder_eval[/\\](?:" + "|".join(re.escape(m) for m in SIDECAR_MODULES) + ")$") + + # Small on purpose: everything here has to exist in whatever interpreter the + # sandbox's shebang resolves to. + STDLIB_ALLOWED = frozenset({"re", "json", "os", "sys", "time", "shlex", "itertools", "typing"}) + + def __init__(self, filepath: str) -> None: + super().__init__(filepath) + self._sidecar = bool(self._SIDECAR.search(filepath)) + + def _check_import(self, node: ast.AST, module: str | None) -> None: + if not self._sidecar or module is None: + return + root = module.split(".")[0] + if root in self.STDLIB_ALLOWED: + return + if root == "__future__": + # Pointing this at STDLIB_ALLOWED would invite the one edit that + # silently retires the rule's own guard on future-import syntax. + self.violation( + node, + f"'{module}' is imported by a module copied beside generated sandbox shims. It is not " + "an import hazard, but the sidecar's import surface is kept minimal and auditable by " + "an allowlist -- drop the line rather than adding '__future__' to STDLIB_ALLOWED.", + ) + return + self.violation( + node, + f"'{module}' is imported by a module copied beside generated sandbox shims, which run " + "where coder_eval and its dependencies are not installed. Use the standard library, or " + f"add '{root}' to CE048's STDLIB_ALLOWED if it really is stdlib.", + ) + + def visit_ImportFrom(self, node: ast.ImportFrom) -> None: + # A relative import (level > 0) is a package import by definition. + self._check_import(node, node.module if node.level == 0 else f".{node.module or ''}") + self.generic_visit(node) + + def visit_Import(self, node: ast.Import) -> None: + for alias in node.names: + self._check_import(node, alias.name) + self.generic_visit(node) diff --git a/tests/lint/runner.py b/tests/lint/runner.py index 092e97a6..020e6dfa 100644 --- a/tests/lint/runner.py +++ b/tests/lint/runner.py @@ -26,6 +26,7 @@ from tests.lint.rules.ce039_config_error_escalates import ConfigErrorEscalates from tests.lint.rules.ce043_no_command_output_truncation import NoCommandOutputTruncation from tests.lint.rules.ce046_env_info_spreads_super import EnvInfoSpreadsSuper +from tests.lint.rules.ce048_sidecar_shim_stdlib_only import SidecarShimStdlibOnly from tests.lint.rules.no_agent_timing_access import NoAgentTimingAccess from tests.lint.rules.no_blocking_io_in_async import NoBlockingIoInAsync from tests.lint.rules.no_cli_imports_in_core import NoCliImportsInCore @@ -75,6 +76,7 @@ ConfigErrorEscalates, NoCommandOutputTruncation, EnvInfoSpreadsSuper, + SidecarShimStdlibOnly, ] # Anti-shadow invariant (mirrors AgentRegistry / register_pricing): every CE rule diff --git a/tests/test_cli_called_criterion.py b/tests/test_cli_called_criterion.py index 8c2d79bb..ce50cee1 100644 --- a/tests/test_cli_called_criterion.py +++ b/tests/test_cli_called_criterion.py @@ -6,9 +6,9 @@ import pytest from pydantic import ValidationError -from coder_eval.criteria.cli_called import _split_flags +from coder_eval.argv_match import split_flags from coder_eval.evaluation.checker import SuccessChecker -from coder_eval.models import CliCalledCriterion, SandboxConfig +from coder_eval.models import CliCalledCriterion, CriterionResult, SandboxConfig from coder_eval.sandbox import Sandbox @@ -174,18 +174,16 @@ def test_dotall_flag_lets_a_pattern_cross_newlines(self, sandbox_with_log): assert checker.check(without_dotall).score == 0.0 assert checker.check(with_dotall).score == 1.0 - def test_invalid_regex_reports_the_offending_flag(self, sandbox_with_log): - sandbox, sandbox_dir = sandbox_with_log - _write_log(sandbox_dir, [_call(["ixp", "projects", "get", "--val", "x"])]) - criterion = CliCalledCriterion( - description="bad pattern", - log=LOG, - verb="ixp projects get", - flags={"val": {"matches_regex": "([unclosed"}}, - ) - result = SuccessChecker(sandbox).check(criterion) - assert result.score == 0.0 - assert "Invalid matches_regex for flag 'val'" in (result.error or "") + def test_invalid_regex_is_refused_at_load_naming_the_flag(self): + """Load-time, not check-time: the same FlagMatch feeds a record_cli response + rule, which evaluates the pattern inside the sandbox and cannot report.""" + with pytest.raises(ValidationError, match="matches_regex is not a valid regex"): + CliCalledCriterion( + description="bad pattern", + log=LOG, + verb="ixp projects get", + flags={"val": {"matches_regex": "([unclosed"}}, + ) def test_absent_distinguishes_missing_from_different_value(self, sandbox_with_log): """`absent` is why flags is a predicate map, not dict[str, str].""" @@ -273,6 +271,136 @@ def test_min_count_requires_repetition(self, sandbox_with_log): class TestLogHandling: + """The five ways this checker refuses to score a log, and why they are UNIFORM. + + All five return a gating 0.0, and none raises. A missing log, a write sentinel, + a `sidecar_error`, an unusable record and a `rule_error` are every one of them + things an AGENT can cause -- the whole recorder directory lives inside the + sandbox it writes to (`rm` the log, fill the disk, delete the matcher beside the + shim, append garbage, append a crafted `rule_error` record). So none of them may + escalate: a `FinalStatus.ERROR` reads as "harness broken, discard this data + point", which is a strictly better outcome for a failing agent than FAILED. + + An earlier revision raised `CheckerMisuseError` on `rule_error`, believing only a + task author could produce it. `test_a_crafted_rule_error_cannot_launder_a_failure` + is the regression test for that. The authoring concern it was addressing is + handled at LOAD time instead, by `RecordedCli._validate_responses_are_evaluable`. + """ + + def test_a_shim_rule_fault_scores_zero_without_escalating(self, sandbox_with_log): + """The log cannot be trusted, so the criterion fails -- but it must not raise. + + Goes through SuccessChecker, not `_check_impl`, so an escalation would + actually propagate here via `handle_criterion_errors`. + """ + sandbox, sandbox_dir = sandbox_with_log + record = _call(["ixp", "dummy1"]) + record["rule_error"] = "TypeError('argument of type int is not iterable')" + _write_log(sandbox_dir, [record]) + + criterion = CliCalledCriterion(description="called dummy1", log=LOG, verb="ixp dummy1") + result = SuccessChecker(sandbox).check(criterion) + assert result.score == 0.0 + assert "could not evaluate its response rules" in (result.error or "") + + def test_a_non_string_rule_error_still_scores_zero(self, sandbox_with_log): + """`parse_log` only validates `argv`, so `rule_error` may be any JSON value. + The message formats it with !r and must not call string methods on it.""" + sandbox, sandbox_dir = sandbox_with_log + record = _call(["ixp", "dummy1"]) + record["rule_error"] = 42 + _write_log(sandbox_dir, [record]) + + criterion = CliCalledCriterion(description="called dummy1", log=LOG, verb="ixp dummy1") + result = SuccessChecker(sandbox).check(criterion) + assert result.score == 0.0 + assert "42" in (result.error or "") + + def test_a_crafted_rule_error_cannot_launder_a_failure(self, sandbox_with_log): + """Regression: `rule_error` used to RAISE, on the premise that only a task + author could cause it. The log is agent-writable, so one appended line turned + an honest FAILED into a FinalStatus.ERROR -- i.e. "discard this data point". + """ + sandbox, sandbox_dir = sandbox_with_log + # The agent never ran the required command, so this must fail. + _write_log(sandbox_dir, []) + criterion = CliCalledCriterion(description="must have called dummy1", log=LOG, verb="ixp dummy1") + assert SuccessChecker(sandbox).check(criterion).score == 0.0 + + log_path = sandbox_dir / LOG + with log_path.open("a", encoding="utf-8") as handle: + handle.write(json.dumps({"argv": [], "rule_error": "TypeError()"}) + "\n") + + # Still a failure, and specifically NOT an exception. + result = SuccessChecker(sandbox).check(criterion) + assert result.score == 0.0, "tampering must not improve the verdict" + assert result.error is not None + + def test_a_rule_error_on_an_unusable_record_takes_the_zero_path(self, sandbox_with_log): + """`faults` is built from `usable` only, so a fault on a record whose argv is + unreadable is counted as unusable instead -- scored 0.0, never raised.""" + sandbox, sandbox_dir = sandbox_with_log + log_path = sandbox_dir / LOG + log_path.parent.mkdir(parents=True, exist_ok=True) + log_path.write_text( + json.dumps({"tool": "uip", "argv": "not-a-list", "rule_error": "TypeError()"}) + "\n", + encoding="utf-8", + ) + criterion = CliCalledCriterion(description="called dummy1", log=LOG, verb="ixp dummy1") + result = SuccessChecker(sandbox).check(criterion) + assert result.score == 0.0 + assert "unusable record" in (result.error or "") + + def test_an_explicit_null_rule_error_is_not_a_fault(self, sandbox_with_log): + """`is not None`, not truthiness: a record carrying `rule_error: null` is a + clean record and must score normally.""" + sandbox, sandbox_dir = sandbox_with_log + record = _call(["ixp", "dummy1"]) + record["rule_error"] = None + _write_log(sandbox_dir, [record]) + + criterion = CliCalledCriterion(description="called dummy1", log=LOG, verb="ixp dummy1") + assert SuccessChecker(sandbox).check(criterion).score == 1.0 + + def test_the_write_failure_sentinel_still_scores_zero(self, sandbox_with_log): + """The one refuse-to-score path with no test before now. + + An agent can cause it (fill the disk, chmod the recorder dir), so it stays a + gating 0.0 rather than joining the escalating path above. + """ + sandbox, sandbox_dir = sandbox_with_log + _write_log(sandbox_dir, [_call(["ixp", "dummy1"])]) + (sandbox_dir / f"{LOG}.error").write_text( + "OSError(28, 'No space left on device') ['ixp', 'dummy2']\n", encoding="utf-8" + ) + + criterion = CliCalledCriterion(description="called dummy1", log=LOG, verb="ixp dummy1") + result = SuccessChecker(sandbox).check(criterion) + assert result.score == 0.0 + assert "could not write" in (result.error or "") + + def test_a_fault_on_one_tool_does_not_fail_another_tools_criterion(self, sandbox_with_log): + """One log serves every shadowed tool, so the fault checks are scoped. + + A `uip` shim that could not import its matcher says nothing about whether + the agent ran `curl`, and failing that guard would report an error message + about response dispatch to an author who never declared a response rule. + """ + sandbox, sandbox_dir = sandbox_with_log + broken = _call(["ixp", "dummy1"], tool="uip") + broken["sidecar_error"] = "ModuleNotFoundError()" + _write_log(sandbox_dir, [broken, _call(["https://example.com"], tool="curl", exit_code=7)]) + + curl = CliCalledCriterion( + description="fetched the url", log=LOG, tool="curl", positional=["https://example.com"], min_count=1 + ) + assert SuccessChecker(sandbox).check(curl).score == 1.0 + + uip = CliCalledCriterion(description="called dummy1", log=LOG, tool="uip", verb="ixp dummy1") + result = SuccessChecker(sandbox).check(uip) + assert result.score == 0.0 + assert "could not import its matcher" in (result.error or "") + def test_missing_log_fails_even_a_negative_guard(self, sandbox_with_log): """A missing log is a harness fault, so `max_count: 0` must NOT pass on it. @@ -322,10 +450,80 @@ def test_malformed_line_now_fails_instead_of_being_skipped(self, sandbox_with_lo assert "1 unusable record" in (result.error or "") +class TestNoEscalationOnAgentControlledContent: + """The log is agent-writable, so NOTHING in it may raise. + + The sensor for the defect this guard was written after: `rule_error` was made to + raise `CheckerMisuseError` on the premise that only a task author could produce + it. `CheckerMisuseError` is in `criteria/base.py::_ESCALATING_EXCEPTIONS`, so it + propagates to `FinalStatus.ERROR`, whose category is "error" and not "failed" -- + the run is discarded instead of counted against the agent. One appended line + bought that. + + A checker over sandbox content has exactly two honest outcomes: a `CriterionResult` + with a score, or a crash that is a coder_eval bug. It may never convert what an + agent WROTE into a harness-fault verdict. Enumerated rather than fuzzed so each + case names the shape it stands for. + """ + + @pytest.mark.parametrize( + ("label", "content"), + [ + ("crafted rule_error", json.dumps({"argv": [], "rule_error": "TypeError()"})), + ("crafted sidecar_error", json.dumps({"argv": [], "sidecar_error": "ImportError()"})), + ("non-string rule_error", json.dumps({"argv": [], "rule_error": {"nested": True}})), + ("rule_error on a real call", json.dumps({"argv": ["ixp", "dummy1"], "rule_error": 1})), + ("both fault keys at once", json.dumps({"argv": [], "rule_error": "a", "sidecar_error": "b"})), + ("not json", "}{ not json at all"), + ("json but not an object", json.dumps([1, 2, 3])), + ("argv not a list", json.dumps({"argv": "ixp dummy1"})), + ("argv not all strings", json.dumps({"argv": ["ixp", 7]})), + ("empty file", ""), + ("only whitespace", " \n\n "), + ("huge argv", json.dumps({"argv": ["x" * 20000]})), + ("surrogates in argv", json.dumps({"argv": ["\udcff"]})), + ], + ) + def test_no_log_content_can_make_the_checker_raise(self, sandbox_with_log, label, content): + sandbox, sandbox_dir = sandbox_with_log + log_path = sandbox_dir / LOG + log_path.parent.mkdir(parents=True, exist_ok=True) + log_path.write_text(content + "\n", encoding="utf-8") + + criterion = CliCalledCriterion(description="called dummy1", log=LOG, verb="ixp dummy1") + result = SuccessChecker(sandbox).check(criterion) + + assert isinstance(result, CriterionResult), f"{label} produced no scored result" + assert result.score in (0.0, 1.0), f"{label} produced a non-binary score" + + def test_a_negative_guard_still_fails_on_every_untrustworthy_log(self, sandbox_with_log): + """The other half: refusing to raise must not become refusing to fail. + + A `max_count: 0` guard passing vacuously on a log the agent damaged is the + mirror-image defect, and the reason all five paths score 0.0 rather than + being skipped. + """ + sandbox, sandbox_dir = sandbox_with_log + log_path = sandbox_dir / LOG + log_path.parent.mkdir(parents=True, exist_ok=True) + forbidden = CliCalledCriterion( + description="must not delete", log=LOG, verb="ixp fields delete", min_count=0, max_count=0 + ) + for content in ( + json.dumps({"argv": [], "rule_error": "TypeError()"}), + json.dumps({"argv": [], "sidecar_error": "ImportError()"}), + "unparseable", + ): + log_path.write_text(content + "\n", encoding="utf-8") + result = SuccessChecker(sandbox).check(forbidden) + assert result.score == 0.0, f"a negative guard passed on: {content}" + assert result.error, "an untrustworthy log must say why" + + class TestArgvNormalization: def test_equals_form_and_space_form_are_equivalent(self): - space = _split_flags(["get", "--model", "pro"], frozenset(), frozenset({"model"})) - equals = _split_flags(["get", "--model=pro"], frozenset(), frozenset({"model"})) + space = split_flags(["get", "--model", "pro"], frozenset(), frozenset({"model"})) + equals = split_flags(["get", "--model=pro"], frozenset(), frozenset({"model"})) assert space == equals == (["get"], {"model": ["pro"]}) def test_output_is_ignored_by_default(self, sandbox_with_log): @@ -344,13 +542,13 @@ def test_output_is_ignored_by_default(self, sandbox_with_log): assert SuccessChecker(sandbox).check(with_json).score == 1.0 def test_boolean_switch_does_not_consume_the_next_flag(self): - positional, flags = _split_flags(["delete", "proj-1", "--yes", "--force"], frozenset(), frozenset()) + positional, flags = split_flags(["delete", "proj-1", "--yes", "--force"], frozenset(), frozenset()) assert positional == ["delete", "proj-1"] assert flags == {"yes": [""], "force": [""]} def test_flag_like_value_stays_a_value(self): """A value that merely looks like a flag is still a value when quoted as one.""" - positional, flags = _split_flags( + positional, flags = split_flags( ["confirm", "--corrections", '[{"v":"--x"}]'], frozenset(), frozenset({"corrections"}) ) assert positional == ["confirm"] @@ -358,13 +556,13 @@ def test_flag_like_value_stays_a_value(self): def test_double_dash_terminates_flag_parsing(self): """`--` is consumed as a separator; what follows is positional, not a flag.""" - positional, flags = _split_flags(["run", "--", "--not-a-flag"], frozenset(), frozenset()) + positional, flags = split_flags(["run", "--", "--not-a-flag"], frozenset(), frozenset()) assert positional == ["run", "--not-a-flag"] assert flags == {} def test_lone_dash_is_positional(self): """A bare `-` is the stdin convention, not a flag.""" - positional, flags = _split_flags(["import", "-"], frozenset(), frozenset()) + positional, flags = split_flags(["import", "-"], frozenset(), frozenset()) assert positional == ["import", "-"] assert flags == {} @@ -461,13 +659,13 @@ def test_clustered_short_flags_are_split(self, sandbox_with_log): def test_declared_multi_char_short_flag_is_taken_whole(self): """Declaring the name wins over splitting, for CLIs with real -ab flags.""" - assert _split_flags(["rm", "-rf", "p"], frozenset(), frozenset(), frozenset({"rf"})) == ( + assert split_flags(["rm", "-rf", "p"], frozenset(), frozenset(), frozenset({"rf"})) == ( ["rm", "p"], {"rf": [""]}, ) def test_attached_value_on_a_short_flag(self): - assert _split_flags(["g", "-ff-002"], frozenset(), frozenset({"f"}), frozenset({"f"})) == ( + assert split_flags(["g", "-ff-002"], frozenset(), frozenset({"f"}), frozenset({"f"})) == ( ["g"], {"f": ["f-002"]}, ) @@ -475,35 +673,35 @@ def test_attached_value_on_a_short_flag(self): def test_bare_negative_number_stays_positional(self): """`-1` as a flag named `1` dropped it from the positionals -- the same silent disappearance as the --yes bug.""" - assert _split_flags(["seek", "-1"], frozenset(), frozenset(), frozenset()) == ( + assert split_flags(["seek", "-1"], frozenset(), frozenset(), frozenset()) == ( ["seek", "-1"], {}, ) - assert _split_flags(["seek", "-1.5"], frozenset(), frozenset(), frozenset())[0] == ["seek", "-1.5"] + assert split_flags(["seek", "-1.5"], frozenset(), frozenset(), frozenset())[0] == ["seek", "-1.5"] def test_declared_numeric_flag_still_parses_as_a_flag(self): """`head -1 file` -- declaring it wins over the numeric rule.""" - assert _split_flags(["head", "-1", "f.txt"], frozenset(), frozenset(), frozenset({"1"})) == ( + assert split_flags(["head", "-1", "f.txt"], frozenset(), frozenset(), frozenset({"1"})) == ( ["head", "f.txt"], {"1": [""]}, ) def test_declared_value_flag_consumes_a_dash_leading_value(self): """`--limit -1 proj-1`: declared value flags bind even a dash-leading value.""" - positional, flags = _split_flags( + positional, flags = split_flags( ["ixp", "proj", "get", "--limit", "-1", "proj-1"], frozenset(), frozenset({"limit"}) ) assert positional == ["ixp", "proj", "get", "proj-1"] assert flags == {"limit": ["-1"]} def test_undeclared_flag_leaves_its_neighbour_positional(self): - positional, flags = _split_flags(["ixp", "fields", "delete", "--yes", "proj-1"], frozenset(), frozenset()) + positional, flags = split_flags(["ixp", "fields", "delete", "--yes", "proj-1"], frozenset(), frozenset()) assert positional == ["ixp", "fields", "delete", "proj-1"] assert flags == {"yes": [""]} def test_equals_form_keeps_a_dash_leading_value_and_invents_no_flag(self): """`--offset=-1` used to drop the value AND invent a flag named `1`.""" - positional, flags = _split_flags(["get", "--offset=-1"], frozenset(), frozenset()) + positional, flags = split_flags(["get", "--offset=-1"], frozenset(), frozenset()) assert positional == ["get"] assert flags == {"offset": ["-1"]} @@ -634,19 +832,15 @@ def test_present_requires_the_flag(self, sandbox_with_log): ) assert SuccessChecker(sandbox).check(criterion).score == 0.0 - def test_bad_regex_flags_value_names_the_flag(self, sandbox_with_log): - """re.error is not a ValueError, so the pre-flight guard missed this.""" - sandbox, sandbox_dir = sandbox_with_log - _write_log(sandbox_dir, [_call(["ixp", "projects", "get", "--val", "x"])]) - criterion = CliCalledCriterion( - description="bad flags int", - log=LOG, - verb="ixp projects get", - flags={"val": {"matches_regex": "a", "flags": 99999999}}, - ) - result = SuccessChecker(sandbox).check(criterion) - assert result.score == 0.0 - assert "flag 'val'" in (result.error or "") + def test_bad_regex_flags_value_is_refused_at_load(self): + """re.error is not a ValueError, so the old pre-flight guard missed this.""" + with pytest.raises(ValidationError, match="not a valid regex with flags=99999999"): + CliCalledCriterion( + description="bad flags int", + log=LOG, + verb="ixp projects get", + flags={"val": {"matches_regex": "a", "flags": 99999999}}, + ) class TestModelValidation: diff --git a/tests/test_cli_match_parity.py b/tests/test_cli_match_parity.py new file mode 100644 index 00000000..90fb3352 --- /dev/null +++ b/tests/test_cli_match_parity.py @@ -0,0 +1,121 @@ +"""Parity between the two argv-matching surfaces. + +``CliMatch`` (a ``record_cli`` response rule's ``when:``) and +``CliCalledCriterion`` declare the same argv facets separately, so each can carry +guidance written for its own job. Separate declarations can drift, and the drift +is silent in the worst direction: a task author stubs a response with a facet the +criterion cannot express, or grades on one no rule can dispatch on, and finds out +only when a suite scores wrong. + +The matching *semantics* cannot drift — both lower to one spec dict that +``coder_eval.argv_match`` evaluates — which is what these tests pin. +""" + +import pytest +from pydantic import ValidationError + +from coder_eval.argv_match import FlagPredicate, MatchSpec, argv_matches +from coder_eval.models import CliCalledCriterion, CliMatch, CliResponse, FlagMatch +from coder_eval.models.cli_match import MATCH_FACET_FIELDS + + +class TestFacetParity: + def test_both_surfaces_declare_every_match_facet(self): + for field in MATCH_FACET_FIELDS: + assert field in CliMatch.model_fields, f"CliMatch is missing match facet {field!r}" + assert field in CliCalledCriterion.model_fields, f"cli_called is missing match facet {field!r}" + + def test_the_rule_surface_declares_no_facet_outside_the_shared_tuple(self): + """Closes the loop the other direction: without this, a facet added to + CliMatch alone passes, since the loop above only walks MATCH_FACET_FIELDS.""" + assert set(CliMatch.model_fields) == set(MATCH_FACET_FIELDS) + + def test_criterion_adds_only_non_argv_fields(self): + """A facet on the criterion that CliMatch lacks is a rule authors cannot write.""" + # Everything the criterion adds is about the LOG (where to read, which + # record, how many), not about the arguments of one invocation. + non_argv = {"log", "tool", "min_count", "max_count"} + base = set(CliCalledCriterion.model_fields) - set(MATCH_FACET_FIELDS) + # Fields inherited from BaseSuccessCriterion are not match surface either. + from coder_eval.models import BaseSuccessCriterion + + added = base - set(BaseSuccessCriterion.model_fields) + assert added == non_argv, f"cli_called gained non-facet field(s) {sorted(added - non_argv)}; add to CliMatch" + + +# One pattern, both surfaces: (pattern, argv, expected verdict). +SHARED_CASES = [ + ({"verb": "ixp dummy1"}, ["ixp", "dummy1"], True), + ({"verb": "ixp dummy1"}, ["ixp", "dummy2"], False), + # Prefix semantics: tokens after the verb are unconstrained. + ({"verb": "ixp dummy1"}, ["ixp", "dummy1", "extra"], True), + # Token-wise, so a longer word never satisfies a shorter one. + ({"verb": "projects list"}, ["projects", "lists"], False), + ({"verb_any_of": ["projects list", "projects get"]}, ["projects", "get", "p1"], True), + ({"positional": ["proj-1"]}, ["proj-1", "tail"], True), + ({"verb": "projects get", "positional": ["proj-1"]}, ["projects", "get", "proj-2"], False), + # Not `output`: the criterion ignores that one by default (see the + # deliberate-divergence test below), so a shared case cannot use it. + ({"verb": "projects get", "flags": {"model": "pro"}}, ["projects", "get", "--model", "pro"], True), + ({"verb": "projects get", "flags": {"model": "pro"}}, ["projects", "get", "--model", "lite"], False), + ({"flags": {"force": {"present": True}}}, ["delete", "--force", "proj-1"], True), + ({"flags": {"force": {"absent": True}}}, ["delete", "proj-1"], True), +] + + +class TestSemanticParity: + """The same pattern, written on either surface, matches the same argv.""" + + @pytest.mark.parametrize(("pattern", "argv", "expected"), SHARED_CASES) + def test_rule_and_criterion_agree(self, pattern, argv, expected): + rule_spec = CliMatch.model_validate(pattern).match_spec + criterion = CliCalledCriterion(description="d", **pattern) + assert argv_matches(rule_spec, argv) is expected + assert argv_matches(criterion.match_spec, argv) is expected + + def test_ignore_flags_default_differs_and_that_is_deliberate(self): + """The criterion drops --output by default; a response rule does not. + + Grading must not depend on a flag that changes nothing about the outcome; + dispatch may legitimately answer differently for `--output json`. + """ + assert CliCalledCriterion(description="d", verb="get").ignore_flags == ["output"] + assert CliMatch(verb="get").ignore_flags == [] + + +class TestSharedValidation: + """One validator, so a pattern rejected on one surface is rejected on both.""" + + @pytest.mark.parametrize("verb", ["ixp projects get --output json", "ixp projects get -o", "delete --yes"]) + def test_a_flag_inside_a_verb_is_rejected_everywhere(self, verb): + """It validated, then matched nothing: the verb is compared to the NON-flag + arguments, so the criterion scored 0 against a log holding that very call and + a response rule fell through to the tool's default.""" + for build in ( + lambda v: CliMatch(verb=v), + lambda v: CliMatch(verb_any_of=[v]), + lambda v: CliCalledCriterion(description="d", verb=v), + lambda v: CliResponse(when={"verb": v}), + ): + with pytest.raises(ValidationError, match="looks like a flag"): + build(verb) + + @pytest.mark.parametrize("verb", ["ixp projects get", "head -1", "seek -1.5"]) + def test_tokens_the_matcher_would_really_see_stay_legal(self, verb): + """`-1` is a value to the splitter, not a flag, so the check must not forbid it.""" + assert CliMatch(verb=verb).verb_spellings == [verb.split()] + assert CliCalledCriterion(description="d", verb=verb).verb_spellings == [verb.split()] + + +class TestLoweredSpecKeys: + """The lowered spec is a TypedDict, so pyright catches a renamed key. These + pin what pyright cannot: that the models and the TypedDicts hold the same + field set, which is what makes `build_match_spec`'s cast honest.""" + + def test_flag_predicate_keys_are_exactly_the_model_fields(self): + assert set(FlagPredicate.__annotations__) == set(FlagMatch.model_fields) + + def test_match_spec_keys_are_exactly_what_lowering_emits(self): + emitted = set(CliMatch(verb="ixp dummy1").match_spec) + assert emitted == set(MatchSpec.__annotations__) + assert emitted == set(CliCalledCriterion(description="d", verb="ixp dummy1").match_spec) diff --git a/tests/test_custom_lint.py b/tests/test_custom_lint.py index adfbd60c..3f4aade9 100644 --- a/tests/test_custom_lint.py +++ b/tests/test_custom_lint.py @@ -146,6 +146,60 @@ def test_ignores_classes_without_the_method(self): assert not self._run("class FooAgent:\n def other(self):\n return {}") +@pytest.mark.lint +class TestCE048SidecarShimStdlibOnly: + """CE048 flags a non-stdlib import in a module copied beside a generated shim.""" + + @staticmethod + def _run(src: str, *, sidecar: bool = True): + import ast + + from tests.lint.rules.ce048_sidecar_shim_stdlib_only import SidecarShimStdlibOnly + + path = "src/coder_eval/argv_match.py" if sidecar else "src/coder_eval/invocation_log.py" + return SidecarShimStdlibOnly(path).check(ast.parse(src)) + + def test_flags_package_import(self): + assert self._run("from coder_eval.models import FlagMatch") + assert self._run("import coder_eval.models") + + def test_flags_third_party_import(self): + assert self._run("import pydantic") + assert self._run("from pydantic import BaseModel") + + def test_flags_relative_import(self): + assert self._run("from .models import FlagMatch") + + def test_flags_a_future_import(self): + """The likeliest accidental addition -- and the one whose generic message + would have been actively misleading, since widening STDLIB_ALLOWED to admit + `__future__` retires the guard instead of fixing the import.""" + violations = self._run("from __future__ import annotations") + assert violations + assert "drop the line" in violations[0].message + + def test_allows_stdlib(self): + assert not self._run("import re\nimport json") + + def test_ignores_files_that_are_not_a_sidecar(self): + # invocation_log.py renders the shim; it is not itself copied beside one. + assert not self._run("from coder_eval.models import RecordedCli", sidecar=False) + + def test_the_rule_guards_a_file_that_actually_exists(self): + """A rule matching nothing passes vacuously while reading as a guarantee -- + which is what a move of the sidecar module would otherwise cause.""" + from pathlib import Path + + from coder_eval.models import SIDECAR_MODULES + from tests.lint.rules.ce048_sidecar_shim_stdlib_only import SidecarShimStdlibOnly + + package = Path(__file__).resolve().parents[1] / "src" / "coder_eval" + for module in SIDECAR_MODULES: + target = package / module + assert target.is_file(), f"SIDECAR_MODULES names {module}, which does not exist" + assert SidecarShimStdlibOnly(str(target))._sidecar, f"CE048 does not match {target}" + + @pytest.mark.lint class TestCE017ModelsLazyAgentImports: """CE017 flags only module-level agents/plugins imports inside models/.""" @@ -1016,6 +1070,41 @@ def test_exemptions_reference_real_fields(self): for field_name in fields: assert field_name in real, f"EXEMPT[{model_name}] names non-field {field_name!r}" + def test_claude_md_names_every_registered_model(self): + """CLAUDE.md carries a prose copy of the CE030 registry, and it had already + gone stale (four names after a sixth was registered). + + Nothing sensed it, because CE030 checks model FIELDS against a doc page, not + its own registry against CLAUDE.md. A prose copy of a registry with no sensor + decays silently, which is the whole failure class CE030 exists for. + """ + from tests.lint.doc_schema_parity import DOCUMENTED_MODELS + + text = (self.REPO_ROOT / "CLAUDE.md").read_text(encoding="utf-8") + sentence = next( + (line for line in text.splitlines() if "models CE030 tracks" in line), + None, + ) + assert sentence, "CLAUDE.md no longer describes CE030's tracked models; update this test" + for model, _ in DOCUMENTED_MODELS: + assert f"`{model.__name__}`" in sentence, ( + f"CLAUDE.md's CE030 list omits {model.__name__}, which is registered in tests/lint/doc_schema_parity.py" + ) + + def test_record_cli_models_are_registered_for_doc_parity(self): + """A future trim of the registry must fail rather than silently drop coverage. + + `RecordedCli` / `CliResponse` are the `record_cli` authoring surface -- the + fields a task author writes by hand -- so an undocumented field on either is + exactly the P0/P1 shape CE030 exists to catch. + """ + from coder_eval.models import CliResponse, RecordedCli + from tests.lint.doc_schema_parity import DOCUMENTED_MODELS + + registered = {model for model, _ in DOCUMENTED_MODELS} + for model in (RecordedCli, CliResponse): + assert model in registered, f"{model.__name__} is no longer registered with CE030" + def test_detects_an_undocumented_field(self): from pydantic import BaseModel, Field diff --git a/tests/test_sandbox_record_cli.py b/tests/test_sandbox_record_cli.py index ef8a9bac..888dc781 100644 --- a/tests/test_sandbox_record_cli.py +++ b/tests/test_sandbox_record_cli.py @@ -10,16 +10,19 @@ import os import subprocess import sys +from pathlib import Path import pytest from pydantic import ValidationError from coder_eval.evaluation.checker import SuccessChecker -from coder_eval.invocation_log import parse_log, render_recorder +from coder_eval.invocation_log import parse_log, render_recorder, sidecar_source from coder_eval.models import ( RECORD_CLI_DIR, RECORD_CLI_LOG, + SIDECAR_MODULES, CliCalledCriterion, + CliResponse, RecordedCli, SandboxConfig, StarterFile, @@ -45,6 +48,18 @@ def _run_shim(sandbox_dir, tool: str, args: list[str]) -> subprocess.CompletedPr ) +# The two rendered shim shapes: the second emits the sidecar import block, the +# first does not. Both are ASCII-only fixtures on purpose (see +# test_rendered_shim_is_pure_ascii). Note neither shape carries argv_match.py's +# body any more -- the sidecar's own "stubs, does not proxy" property is NOT +# covered by the invariants asserted over these two shapes; CE048 covers its +# imports, and nothing covers its exec surface. +SHIM_SHAPES = ( + RecordedCli(tool="uip"), + RecordedCli(tool="uip", responses=[CliResponse(when={"verb": "ixp dummy1"}, stdout="ok")]), +) + + def _records(text: str) -> list[dict]: """Just the records; parse_log also returns the unusable count.""" usable, _ = parse_log(text) @@ -472,15 +487,42 @@ def test_rendered_shim_is_valid_python_and_embeds_config(self): assert namespace["EXIT_CODE"] == 3 assert namespace["STDERR_TEXT"] == "boom\n" - def test_rendered_shim_does_not_execute_anything(self): - """It stubs a tool rather than proxying one: no subprocess, no exec.""" - source = render_recorder(RecordedCli(tool="uip")) + @pytest.mark.parametrize("spec", SHIM_SHAPES, ids=("no_rules", "with_rules")) + def test_rendered_shim_does_not_execute_anything(self, spec): + """It stubs a tool rather than proxying one: no subprocess, no exec. + + Covers the TEMPLATE only. The sidecar the rules-bearing shape imports is + a separate file and is not asserted here. + """ + source = render_recorder(spec) for forbidden in ("subprocess", "execv", "execvp", "popen", "system("): assert forbidden not in source - def test_rendered_shim_imports_nothing_from_coder_eval(self): - """It runs inside the sandbox, where this package is not installed.""" - source = render_recorder(RecordedCli(tool="uip")) + @pytest.mark.parametrize("module", SIDECAR_MODULES) + def test_the_sidecar_does_not_execute_anything_either(self, module): + """Restores coverage the sidecar refactor silently dropped. + + While `argv_match.py` was SPLICED into the shim, + `test_rendered_shim_does_not_execute_anything` scanned its body too. As a + separate file it is no longer in that scan, and CE048 cannot stand in: + `os` is on its STDLIB_ALLOWED (the matcher genuinely needs it), so + `os.system(...)` in the sidecar would pass lint, typecheck, and ship into + every sandbox. "It stubs a tool; it does not proxy one" is a documented + promise in docs/TASK_DEFINITION_GUIDE.md -- this is what keeps it true for + the half most likely to grow. + """ + source = sidecar_source(module) + for forbidden in ("subprocess", "execv", "execvp", "popen", "system("): + assert forbidden not in source, f"{module} reaches a subprocess via {forbidden!r}" + + @pytest.mark.parametrize("spec", SHIM_SHAPES, ids=("no_rules", "with_rules")) + def test_rendered_shim_imports_nothing_from_coder_eval(self, spec): + """It runs inside the sandbox, where this package is not installed. + + `from argv_match import select_rule` is a SIBLING import of the sidecar + written beside the shim, not a package import, so it does not appear here. + """ + source = render_recorder(spec) imports = [ line.strip() for line in source.splitlines() @@ -488,9 +530,16 @@ def test_rendered_shim_imports_nothing_from_coder_eval(self): ] assert imports == [] - def test_rendered_shim_is_pure_ascii(self): - """Written into arbitrary sandboxes and read by whatever python3 is there.""" - source = render_recorder(RecordedCli(tool="uip")) + @pytest.mark.parametrize("spec", SHIM_SHAPES, ids=("no_rules", "with_rules")) + def test_rendered_shim_is_pure_ascii(self, spec): + """Written into arbitrary sandboxes and read by whatever python3 is there. + + Scoped to the ASCII-only SHIM_SHAPES fixtures: author-supplied `stdout` / + `stderr` may legitimately be any UTF-8, so only the TEMPLATE is + ASCII-constrained. Both shapes are rendered because only the second emits + the sidecar import block. + """ + source = render_recorder(spec) source.encode("ascii") def test_parse_log_separates_usable_from_unusable(self): @@ -504,3 +553,630 @@ def test_parse_log_separates_usable_from_unusable(self): assert [argv for argv, _ in usable] == [["a"]] # An argv that is not list[str] is unusable, not a non-match. assert unusable == 2 + + +class TestSidecarModule: + """The matcher reaches the shim as a SIBLING FILE, not as spliced source. + + A shim that carries a copy of `argv_match.py` in its own namespace is one + accidental name collision away from every invocation silently falling back to + the entry defaults -- `respond()` swallows the resulting TypeError. Writing the + module beside the shim and importing it removes that class of failure, at the + cost of one more file the recorder directory must contain: these tests are what + keep that file actually landing there. + """ + + @staticmethod + def _spec_with_rules(tool: str = "uip") -> RecordedCli: + return RecordedCli( + tool=tool, + exit_code=1, + stderr="uip: unknown command\n", + responses=[ + CliResponse(when={"verb": "ixp dummy1"}, stdout="response1\n"), + CliResponse(when={"verb": "ixp dummy2"}, stdout="response2\n"), + ], + ) + + def test_sidecar_lands_beside_a_rules_bearing_shim(self): + sandbox = _sandbox("sidecar_present", record_cli=[self._spec_with_rules()]) + try: + recorder_dir = sandbox.setup() / RECORD_CLI_DIR + for module in SIDECAR_MODULES: + assert (recorder_dir / module).is_file(), f"{module} was not written beside the shim" + finally: + sandbox.cleanup(preserve=False) + + def test_no_sidecar_without_rules(self): + """A shim that answers everything the same way never consults the matcher.""" + sandbox = _sandbox("sidecar_absent", record_cli=[RecordedCli(tool="uip")]) + try: + recorder_dir = sandbox.setup() / RECORD_CLI_DIR + for module in SIDECAR_MODULES: + assert not (recorder_dir / module).exists() + finally: + sandbox.cleanup(preserve=False) + + def test_sidecar_is_the_shipped_source_verbatim(self): + """Not a paraphrase: the shim's matcher IS coder_eval/argv_match.py.""" + from coder_eval import argv_match + + shipped = Path(argv_match.__file__).read_text(encoding="utf-8") + sandbox = _sandbox("sidecar_verbatim", record_cli=[self._spec_with_rules()]) + try: + written = (sandbox.setup() / RECORD_CLI_DIR / "argv_match.py").read_text(encoding="utf-8") + assert written == shipped + finally: + sandbox.cleanup(preserve=False) + + def test_a_rules_bearing_shim_dispatches_through_the_sidecar(self): + """End to end: the sibling import resolves, so per-rule dispatch works. + + Every other assertion in this class is about a file existing. This one is + the proof the shim can actually IMPORT it from inside the sandbox. + """ + sandbox = _sandbox("sidecar_dispatch", record_cli=[self._spec_with_rules()]) + try: + sandbox_dir = sandbox.setup() + first = _run_shim(sandbox_dir, "uip", ["ixp", "dummy1"]) + second = _run_shim(sandbox_dir, "uip", ["ixp", "dummy2"]) + fallback = _run_shim(sandbox_dir, "uip", ["ixp", "nope"]) + + assert (first.returncode, first.stdout) == (0, "response1\n") + assert (second.returncode, second.stdout) == (0, "response2\n") + assert (fallback.returncode, fallback.stdout) == (1, "") + assert "unknown command" in fallback.stderr + # An ImportError would land here rather than on the exit code, since + # the shim would die before writing anything. + assert "ImportError" not in first.stderr + + records = _records((sandbox_dir / RECORD_CLI_LOG).read_text(encoding="utf-8")) + assert [record.get("rule") for record in records] == [0, 1, None] + finally: + sandbox.cleanup(preserve=False) + + def test_the_template_is_what_suppresses_pycache(self, tmp_path): + """`sys.dont_write_bytecode = True` above the import, not the environment. + + The sibling import would otherwise create `cli_mocks/__pycache__/` inside + the agent's sandbox -- a directory the agent can see and that a file_check + criterion or an artifact diff picks up. The negative half is load-bearing: + docker/Dockerfile already sets PYTHONDONTWRITEBYTECODE, and CI runners often + do too, so without it this test would pass wherever the template line was + deleted. + """ + from coder_eval import argv_match + + source = render_recorder(self._spec_with_rules()) + stripped = source.replace("sys.dont_write_bytecode = True\n", "", 1) + assert stripped != source, "the suppression line moved; update this test" + + # PYTHONDONTWRITEBYTECODE would mask the template line in BOTH halves; + # PYTHONPYCACHEPREFIX would send the negative half's bytecode to a shadow + # tree and fail it spuriously. Neither may leak in from the developer's env. + env = {k: v for k, v in os.environ.items() if k not in ("PYTHONDONTWRITEBYTECODE", "PYTHONPYCACHEPREFIX")} + + for name, shim_source, expect_pycache in (("with", source, False), ("without", stripped, True)): + work = tmp_path / name + work.mkdir() + (work / "argv_match.py").write_text(Path(argv_match.__file__).read_text(encoding="utf-8"), encoding="utf-8") + shim = work / "uip" + shim.write_text(shim_source, encoding="utf-8", newline="\n") + proc = subprocess.run( + [sys.executable, str(shim), "ixp", "dummy1"], + capture_output=True, + text=True, + encoding="utf-8", + env=env, + check=False, + ) + assert proc.stdout == "response1\n", f"the {name} shim did not dispatch: {proc.stderr}" + assert (work / "__pycache__").exists() is expect_pycache + + def test_a_rules_less_and_a_rules_bearing_entry_coexist(self): + """One sandbox, one sidecar, two shims -- only one of which imports it.""" + sandbox = _sandbox( + "sidecar_mixed", + record_cli=[self._spec_with_rules(), RecordedCli(tool="curl", exit_code=7)], + ) + try: + sandbox_dir = sandbox.setup() + assert (sandbox_dir / RECORD_CLI_DIR / "argv_match.py").is_file() + assert _run_shim(sandbox_dir, "uip", ["ixp", "dummy1"]).stdout == "response1\n" + assert _run_shim(sandbox_dir, "curl", ["https://example.com"]).returncode == 7 + records = _records((sandbox_dir / RECORD_CLI_LOG).read_text(encoding="utf-8")) + assert [record["tool"] for record in records] == ["uip", "curl"] + finally: + sandbox.cleanup(preserve=False) + + def test_a_second_rules_bearing_entry_does_not_collide_on_the_sidecar(self): + """Identical bytes, so a second write is not a collision -- which is why the + per-tool `exists()` pre-check was NOT widened to cover the sidecar. + + Asserts the OUTCOME (no raise, correct content, both shims present), not a + write count: rewriting the same bytes N times is indistinguishable here and + is equally correct. + """ + from coder_eval import argv_match + + sandbox = _sandbox( + "sidecar_twice", + record_cli=[self._spec_with_rules("uip"), self._spec_with_rules("aip")], + ) + try: + recorder_dir = sandbox.setup() / RECORD_CLI_DIR + assert (recorder_dir / "argv_match.py").read_text(encoding="utf-8") == Path(argv_match.__file__).read_text( + encoding="utf-8" + ) + assert (recorder_dir / "uip").is_file() + assert (recorder_dir / "aip").is_file() + finally: + sandbox.cleanup(preserve=False) + + def test_a_re_setup_does_not_leave_a_stale_sidecar(self, tmp_path): + """The recorder dir is wiped every setup, so a sidecar cannot outlive the + entry that asked for it. + + Uses an explicit `target_dir`, like the two stale-state tests above: a + default tempdir sandbox gets a FRESH mkdtemp per setup(), so it never + re-enters the `shutil.rmtree(recorder_dir)` branch this is about, and the + assertion would hold with that branch deleted. + """ + target = tmp_path / "artifacts" + with_rules = _sandbox("sidecar_resetup", record_cli=[self._spec_with_rules()]) + with_rules.setup(target_dir=target) + assert (target / RECORD_CLI_DIR / "argv_match.py").is_file() + + plain = _sandbox("sidecar_resetup", record_cli=[RecordedCli(tool="uip")]) + plain.setup(target_dir=target) + assert not (target / RECORD_CLI_DIR / "argv_match.py").exists(), ( + "the previous setup's sidecar survived into a run that declares no rules" + ) + + def test_a_broken_sidecar_still_records_the_invocation(self): + """A shim whose matcher will not import must NOT become a tool that runs and + records nothing. + + Splicing made this state unreachable -- there was no import to fail. An + empty log reads exactly like "the agent never called it", which is the one + reading `cli_called` works hardest to prevent (the log is seeded so missing + and empty differ, a sentinel covers dropped writes, unusable records are + counted). So the import is guarded and the fault is booked on every record. + """ + sandbox = _sandbox("sidecar_broken", record_cli=[self._spec_with_rules()]) + try: + sandbox_dir = sandbox.setup() + (sandbox_dir / RECORD_CLI_DIR / "argv_match.py").unlink() + + proc = _run_shim(sandbox_dir, "uip", ["ixp", "dummy1"]) + # The entry defaults, not the rule's response: the matcher is gone. + assert proc.returncode == 1 + assert proc.stdout == "" + + record = _records((sandbox_dir / RECORD_CLI_LOG).read_text(encoding="utf-8"))[0] + assert record["argv"] == ["ixp", "dummy1"], "the invocation went unrecorded" + # FileNotFoundError, not ModuleNotFoundError: the sidecar is loaded by + # absolute path, so a missing file never reaches the import machinery. + assert "FileNotFoundError" in record["sidecar_error"] + assert "rule" not in record + finally: + sandbox.cleanup(preserve=False) + + def test_a_broken_sidecar_cannot_let_a_negative_guard_pass(self): + """The consequence that makes the guard above load-bearing. + + With the invocation unrecorded, `max_count: 0` over the very call the task + forbids scored 1.0 with no error -- a silent false PASS. + """ + sandbox = _sandbox("sidecar_broken_guard", record_cli=[self._spec_with_rules()]) + try: + sandbox_dir = sandbox.setup() + (sandbox_dir / RECORD_CLI_DIR / "argv_match.py").unlink() + _run_shim(sandbox_dir, "uip", ["ixp", "dummy1"]) + + forbidden = CliCalledCriterion( + description="must not call dummy1", verb="ixp dummy1", min_count=0, max_count=0 + ) + result = SuccessChecker(sandbox).check(forbidden) + assert result.score == 0.0 + assert "matcher" in (result.error or "").lower() or "sidecar" in (result.error or "").lower() + + # And a POSITIVE criterion must not read as a clean pass either: the + # agent saw the fallback, not the response the task described. + wanted = CliCalledCriterion(description="called dummy1", verb="ixp dummy1", min_count=1) + assert SuccessChecker(sandbox).check(wanted).score == 0.0 + finally: + sandbox.cleanup(preserve=False) + + def test_the_recorder_dir_does_not_shadow_the_sidecars_own_stdlib_imports(self): + """argv_match.py imports `re` and `typing`, and the recorder dir is writable + by the agent AND holds a shim per declared tool. With that directory at the + HEAD of sys.path, a tool named `typing.py` broke every rules-bearing shim in + the sandbox -- so the sidecar dir is appended, not prepended. + """ + sandbox = _sandbox( + "sidecar_shadow", + record_cli=[RecordedCli(tool="typing.py"), self._spec_with_rules()], + ) + try: + sandbox_dir = sandbox.setup() + proc = _run_shim(sandbox_dir, "uip", ["ixp", "dummy1"]) + assert (proc.returncode, proc.stdout) == (0, "response1\n"), f"shadowed: {proc.stderr}" + finally: + sandbox.cleanup(preserve=False) + + @pytest.mark.parametrize("via", ["pythonpath", "cwd"]) + def test_an_unrelated_argv_match_earlier_on_sys_path_cannot_hijack_dispatch(self, tmp_path, via): + """The sidecar is loaded by ABSOLUTE PATH, not resolved by name. + + A plain `import argv_match` obeys sys.path order, so an unrelated (or + planted) argv_match in the cwd, on PYTHONPATH, or in site-packages would + win over the file written beside the shim -- and silently, if it happens to + export `select_rule`. Then every canned response the task described is + replaced by whatever that module returns. + """ + impostor = tmp_path / "elsewhere" + impostor.mkdir() + (impostor / "argv_match.py").write_text( + "def select_rule(rules, argv):\n return (0, {'exit': 0, 'stdout': 'HIJACKED\\n', 'stderr': ''})\n", + encoding="utf-8", + ) + + sandbox = _sandbox(f"sidecar_hijack_{via}", record_cli=[self._spec_with_rules()]) + try: + sandbox_dir = sandbox.setup() + env = {k: v for k, v in os.environ.items() if k != "PYTHONDONTWRITEBYTECODE"} + if via == "pythonpath": + env["PYTHONPATH"] = str(impostor) + proc = subprocess.run( + [sys.executable, str(sandbox_dir / RECORD_CLI_DIR / "uip"), "ixp", "dummy1"], + capture_output=True, + text=True, + encoding="utf-8", + cwd=str(impostor) if via == "cwd" else None, + env=env, + check=False, + ) + assert proc.stdout == "response1\n", f"dispatch was hijacked via {via}: {proc.stdout!r}" + record = _records((sandbox_dir / RECORD_CLI_LOG).read_text(encoding="utf-8"))[0] + assert "sidecar_error" not in record + assert record["rule"] == 0 + finally: + sandbox.cleanup(preserve=False) + + def test_the_sidecar_import_survives_pythonsafepath(self): + """PYTHONSAFEPATH=1 clears sys.path[0] -- the entire reason SHIM_DIR is put on + the path explicitly. Asserted at RUNTIME, not just textually.""" + sandbox = _sandbox("sidecar_safepath", record_cli=[self._spec_with_rules()]) + try: + sandbox_dir = sandbox.setup() + proc = subprocess.run( + [sys.executable, str(sandbox_dir / RECORD_CLI_DIR / "uip"), "ixp", "dummy1"], + capture_output=True, + text=True, + encoding="utf-8", + cwd=os.path.dirname(os.path.abspath(os.sep)), + env={**os.environ, "PYTHONSAFEPATH": "1"}, + check=False, + ) + assert (proc.returncode, proc.stdout) == (0, "response1\n"), f"safepath broke it: {proc.stderr}" + finally: + sandbox.cleanup(preserve=False) + + def test_an_unevaluable_response_rule_is_rejected_at_load(self, monkeypatch): + """The authoring fault that `rule_error` used to escalate for, caught where + the agent cannot participate. + + `cli_called` can only score a `rule_error` 0.0 -- the log is agent-writable, + so a fault there cannot be attributed to the task author. Attribution has to + happen before a sandbox exists, so `RecordedCli` runs the real matcher over + every rule at load time. + """ + from coder_eval.models import cli_match + + original = cli_match.CliMatch.match_spec.fget + assert original is not None + # Stand in for any spec the matcher cannot evaluate. Reached in production + # only by a coder_eval bug, which is precisely why nothing else covers it. + monkeypatch.setattr( + cli_match.CliMatch, + "match_spec", + property(lambda self: {**original(self), "verb_spellings": 5}), + ) + with pytest.raises(ValidationError, match="cannot be evaluated"): + RecordedCli(tool="uip", responses=[CliResponse(when={"verb": "ixp dummy1"})]) + + def test_evaluable_rules_are_not_rejected(self): + """The load-time guard must not over-reach: every shape the authoring surface + accepts has to survive it.""" + spec = RecordedCli( + tool="uip", + responses=[ + CliResponse(when={"verb": "ixp dummy1"}, stdout="a"), + CliResponse(when={"verb_any_of": ["ixp x", "ixp y"]}, stdout="b"), + CliResponse( + when={"verb": "ixp projects get", "positional": ["p1"], "flags": {"model": "pro"}}, + stdout="c", + ), + CliResponse(when={"positional": ["bare"]}, stdout="d"), + ], + ) + assert len(spec.responses) == 4 + + @pytest.mark.parametrize("tool", ["argv_match.py", "ARGV_MATCH.PY"]) + def test_a_tool_named_like_a_sidecar_is_rejected(self, tool): + """Case-folded: APFS and NTFS are case-insensitive, so the sidecar write + would clobber the agent's shim without the per-tool exists() guard firing.""" + with pytest.raises(ValidationError, match="collides with a module"): + RecordedCli(tool=tool) + + def test_a_tool_named_like_a_sidecar_without_the_extension_is_allowed(self): + """An extensionless file is not importable, so it cannot shadow the sidecar. + The guard must not over-reach into names that are safe.""" + assert RecordedCli(tool="argv_match").tool == "argv_match" + + +class TestPerInvocationResponses: + """`responses:` — one shadowed tool answering each subcommand differently. + + The reason the shim is more than a recorder: an agent that reads + `ixp projects list` and acts on what came back cannot be evaluated by a stub + that returns the same line for everything it types. + """ + + @staticmethod + def _spec() -> RecordedCli: + return RecordedCli( + tool="uip", + exit_code=1, + stderr="uip: unknown command\n", + responses=[ + CliResponse(when={"verb": "ixp dummy1"}, stdout="response1\n"), + CliResponse(when={"verb": "ixp dummy2"}, stdout="response2\n"), + ], + ) + + def test_each_verb_gets_its_own_response(self): + sandbox = _sandbox("record_responses", record_cli=[self._spec()]) + try: + sandbox_dir = sandbox.setup() + first = _run_shim(sandbox_dir, "uip", ["ixp", "dummy1"]) + second = _run_shim(sandbox_dir, "uip", ["ixp", "dummy2"]) + assert (first.returncode, first.stdout) == (0, "response1\n") + assert (second.returncode, second.stdout) == (0, "response2\n") + finally: + sandbox.cleanup(preserve=False) + + def test_unmatched_invocation_falls_back_to_the_entry_defaults(self): + sandbox = _sandbox("record_responses_fallback", record_cli=[self._spec()]) + try: + sandbox_dir = sandbox.setup() + proc = _run_shim(sandbox_dir, "uip", ["ixp", "dummy3"]) + assert proc.returncode == 1 + assert proc.stdout == "" + assert "unknown command" in proc.stderr + finally: + sandbox.cleanup(preserve=False) + + def test_log_names_the_rule_that_answered(self): + """ "Returned the default" and "rule 1 answered" are otherwise the same line.""" + sandbox = _sandbox("record_responses_log", record_cli=[self._spec()]) + try: + sandbox_dir = sandbox.setup() + _run_shim(sandbox_dir, "uip", ["ixp", "dummy2"]) + _run_shim(sandbox_dir, "uip", ["ixp", "dummy3"]) + records = _records((sandbox_dir / RECORD_CLI_LOG).read_text(encoding="utf-8")) + assert records[0]["rule"] == 1 + assert records[0]["exit"] == 0 + assert "rule" not in records[1], "no rule matched, so none may be claimed" + assert records[1]["exit"] == 1 + finally: + sandbox.cleanup(preserve=False) + + def test_first_matching_rule_wins(self): + """Order is the author's disambiguation tool, so the general rule last.""" + spec = RecordedCli( + tool="uip", + responses=[ + CliResponse(when={"verb": "ixp projects get proj-1"}, stdout="specific\n"), + CliResponse(when={"verb": "ixp projects get"}, stdout="generic\n"), + ], + ) + sandbox = _sandbox("record_responses_order", record_cli=[spec]) + try: + sandbox_dir = sandbox.setup() + assert _run_shim(sandbox_dir, "uip", ["ixp", "projects", "get", "proj-1"]).stdout == "specific\n" + assert _run_shim(sandbox_dir, "uip", ["ixp", "projects", "get", "proj-9"]).stdout == "generic\n" + finally: + sandbox.cleanup(preserve=False) + + def test_rule_can_match_on_flags_and_positional(self): + spec = RecordedCli( + tool="uip", + responses=[ + CliResponse( + when={"verb": "ixp projects get", "positional": ["proj-1"], "flags": {"output": "json"}}, + stdout='{"id": "proj-1"}', + ), + CliResponse(when={"verb": "ixp projects get"}, stdout="proj-1 (table)\n"), + ], + ) + sandbox = _sandbox("record_responses_flags", record_cli=[spec]) + try: + sandbox_dir = sandbox.setup() + asked_json = _run_shim(sandbox_dir, "uip", ["ixp", "projects", "get", "proj-1", "--output", "json"]) + asked_table = _run_shim(sandbox_dir, "uip", ["ixp", "projects", "get", "proj-1"]) + other_project = _run_shim(sandbox_dir, "uip", ["ixp", "projects", "get", "proj-2", "--output", "json"]) + assert asked_json.stdout == '{"id": "proj-1"}' + assert asked_table.stdout == "proj-1 (table)\n" + assert other_project.stdout == "proj-1 (table)\n" + finally: + sandbox.cleanup(preserve=False) + + def test_stderr_and_exit_code_are_per_rule(self): + spec = RecordedCli( + tool="uip", + exit_code=0, + responses=[CliResponse(when={"verb": "ixp projects get missing"}, exit_code=4, stderr="not found\n")], + ) + sandbox = _sandbox("record_responses_failure", record_cli=[spec]) + try: + sandbox_dir = sandbox.setup() + proc = _run_shim(sandbox_dir, "uip", ["ixp", "projects", "get", "missing"]) + assert (proc.returncode, proc.stderr) == (4, "not found\n") + # The entry default still applies to everything else, including its 0. + assert _run_shim(sandbox_dir, "uip", ["ixp", "projects", "list"]).returncode == 0 + finally: + sandbox.cleanup(preserve=False) + + def test_the_pattern_that_served_the_response_also_grades_it(self): + """One semantic across both surfaces: same facets, same verdict. + + A rule and a criterion written from the same pattern must agree, or a task + stubs one invocation and grades another. + """ + pattern = {"verb": "ixp projects configure-model", "positional": ["proj-1"], "flags": {"model": "pro"}} + spec = RecordedCli(tool="uip", responses=[CliResponse(when=dict(pattern), stdout="ok\n")]) + sandbox = _sandbox("record_responses_parity", record_cli=[spec]) + try: + sandbox_dir = sandbox.setup() + served = _run_shim(sandbox_dir, "uip", ["ixp", "projects", "configure-model", "proj-1", "--model", "pro"]) + assert served.stdout == "ok\n", "the rule did not match, so the grading half proves nothing" + criterion = CliCalledCriterion(description="configured the model", **pattern) + assert SuccessChecker(sandbox).check(criterion).score == 1.0 + finally: + sandbox.cleanup(preserve=False) + + def test_a_rule_evaluation_fault_is_recorded_and_fails_the_grading(self): + """The shim swallows a matcher fault so the stub does not crash, but the + record must say so: without it, an eval-config fault is byte-identical to a + legitimate no-match and the task scores as if the agent never made the call. + + FlagMatch compiles at load, so the only way to reach this is to corrupt a + rendered shim -- which is the point: the branch is defense in depth, and + nothing else exercises it. + """ + sandbox = _sandbox("record_rule_fault", record_cli=[self._spec()]) + try: + sandbox_dir = sandbox.setup() + shim = sandbox_dir / RECORD_CLI_DIR / "uip" + source = shim.read_text(encoding="utf-8") + # A spec no matcher can evaluate, standing in for any future shim fault. + broken = source.replace("'verb_spellings': [['ixp', 'dummy1']]", "'verb_spellings': 5", 1) + assert broken != source, "the rule literal moved; update this test" + shim.write_text(broken, encoding="utf-8") + + proc = _run_shim(sandbox_dir, "uip", ["ixp", "dummy1"]) + assert proc.returncode == 1, "the stub must still answer, not crash" + assert "response matching failed" in proc.stderr + + record = _records((sandbox_dir / RECORD_CLI_LOG).read_text(encoding="utf-8"))[0] + assert "rule" not in record + assert "TypeError" in record["rule_error"] + + # Scores 0.0 and does NOT escalate: this test reaches the state by + # editing the shim, which is exactly what an agent can also do, so an + # escalation here would be a FinalStatus.ERROR an agent could trigger + # at will. The assertions above are about the SHIM and are unchanged. + criterion = CliCalledCriterion(description="called dummy1", verb="ixp dummy1") + result = SuccessChecker(sandbox).check(criterion) + assert result.score == 0.0 + assert "could not evaluate its response rules" in (result.error or "") + finally: + sandbox.cleanup(preserve=False) + + def test_rendered_shim_imports_the_sidecar_only_when_rules_exist(self): + """A shim with no rules never consults the matcher, so it does not import it. + + Neither shape may carry the matcher's BODY: the whole point of the sidecar + is that the shim references a sibling file instead of a spliced copy. + """ + plain = render_recorder(RecordedCli(tool="uip")) + with_rules = render_recorder(RecordedCli(tool="uip", responses=[CliResponse(when={"verb": "ixp dummy1"})])) + assert "argv_match.py" not in plain + assert "argv_match.py" in with_rules + assert "select_rule = _sidecar.select_rule" not in plain + assert "select_rule = _sidecar.select_rule" in with_rules + assert "def argv_matches" not in plain + assert "def argv_matches" not in with_rules + compile(plain, "shim", "exec") + compile(with_rules, "shim", "exec") + + def test_the_shim_dir_is_put_on_sys_path_before_the_sidecar_import(self): + """SHIM_DIR must be assigned ABOVE the import block that reads it, and the + path must be set up before the import runs. + + The runtime proof is + TestSidecarModule::test_the_sidecar_import_survives_pythonsafepath; this + pins the ordering the template depends on, which a reordering edit would + otherwise break only under PYTHONSAFEPATH=1. + """ + source = render_recorder(RecordedCli(tool="uip", responses=[CliResponse(when={"verb": "ixp dummy1"})])) + assigned = source.index("SHIM_DIR = os.path.dirname") + pruned = source.index("sys.path[:] = ") + loaded = source.index("spec_from_file_location") + assert assigned < pruned < loaded + # The recorder dir is agent-writable, so it must never sit on sys.path + # while the sidecar executes -- a `typing.py` shim there would shadow the + # matcher's own stdlib imports. + assert "sys.path.insert(0, SHIM_DIR)" not in source + assert "sys.path.append(SHIM_DIR)" not in source + # And the sidecar is never resolved by NAME, which sys.path order decides. + assert "from argv_match import" not in source + + def test_response_rule_needs_a_facet(self): + """A catch-all rule is the entry's own default; two ways to say it is one too many.""" + with pytest.raises(ValidationError, match="at least one of verb"): + CliResponse(when={}) + + @pytest.mark.parametrize( + ("responses", "expected"), + [ + ([{"when": {"verb": "ixp x"}, "stdout": "a"}, {"when": {"verb": "ixp x"}, "stdout": "b"}], "duplicate"), + ([{"when": {"verb": "ixp projects"}}, {"when": {"verb": "ixp projects get"}}], "already claimed"), + ( + [{"when": {"verb": "ixp projects"}}, {"when": {"verb_any_of": ["ixp projects get", "ixp projects x"]}}], + "already claimed", + ), + ], + ids=("exact_duplicate", "general_above_specific", "every_alternative_covered"), + ) + def test_a_rule_an_earlier_rule_already_claims_is_rejected(self, responses, expected): + """First-match-wins makes such a rule dead, and the rest of this surface + hard-errors on every declaration that cannot take effect.""" + with pytest.raises(ValidationError, match=expected): + RecordedCli(tool="uip", responses=responses) + + @pytest.mark.parametrize( + "responses", + [ + [{"when": {"verb": "ixp projects get"}}, {"when": {"verb": "ixp projects"}}], + [{"when": {"verb": "ixp projects", "flags": {"o": "j"}}}, {"when": {"verb": "ixp projects get"}}], + [{"when": {"verb": "ixp projects", "positional": ["p1"]}}, {"when": {"verb": "ixp projects get"}}], + [{"when": {"verb": "ixp projects", "value_flags": []}}, {"when": {"verb": "ixp projects get"}}], + [{"when": {"verb": "ixp a"}}, {"when": {"verb": "ixp b"}}], + # A predicate makes its flag known and value-bearing in the LATER + # rule's parse only: `--profile prod ixp projects get` leaves `prod` + # positional for the verb-only rule, which therefore does not match. + [{"when": {"verb": "ixp projects"}}, {"when": {"verb": "ixp projects get", "flags": {"profile": "p"}}}], + ], + ids=( + "specific_first", + "general_has_flag", + "general_has_positional", + "parsing_differs", + "unrelated", + "later_flag_predicate_changes_parsing", + ), + ) + def test_a_reachable_rule_is_not_rejected(self, responses): + """The check must stay narrow: an earlier rule that constrains anything + beyond its verb does NOT claim everything a later rule would, and two + rules parsing argv differently cannot be compared by verb prefix at all.""" + assert len(RecordedCli(tool="uip", responses=responses).responses) == 2 + + def test_a_bare_string_when_is_rejected_with_the_fix(self): + """One shape for a pattern. A lone string leaves which of six facets it sets + to inference, and reads enough like a command line to invite flags.""" + with pytest.raises(ValidationError, match=r'use \{verb: "ixp dummy1"\}'): + CliResponse(when="ixp dummy1") diff --git a/tests/test_tags.py b/tests/test_tags.py index 54f374e0..e49f921c 100644 --- a/tests/test_tags.py +++ b/tests/test_tags.py @@ -1,13 +1,16 @@ """Tests for task tagging and tag-based filtering.""" import re +import subprocess +import sys from pathlib import Path import pytest import yaml -from coder_eval.models import TaskDefinition +from coder_eval.models import RECORD_CLI_DIR, RECORD_CLI_LOG, SandboxConfig, TaskDefinition from coder_eval.orchestration.batch import filter_tasks_by_tags +from coder_eval.sandbox import Sandbox def _make_task(task_id: str, tags: list[str]) -> TaskDefinition: @@ -210,6 +213,188 @@ def test_makefile_smoke_globs_match_the_ci_globs(self): ) +class TestTasksReadmeSmokeMembers: + """tasks/README.md calls itself "the map", and its smoke Members list had no sensor. + + It had already decayed (`opencode_smoke_test` was tagged `smoke` and missing) -- + the same silent-decay class `TestCiSmokePassContract` guards for the CI counts. + """ + + README = Path("tasks/README.md") + + def test_every_root_smoke_task_is_listed(self): + if not self.README.exists(): + pytest.skip("tasks/README.md not present") + tagged = set() + for task_file in sorted(Path("tasks").glob("*.yaml")): + try: + task = TaskDefinition(**yaml.safe_load(task_file.read_text(encoding="utf-8"))) + except Exception: # malformed tasks are other tests' business + continue + if any(tag.startswith("smoke") for tag in task.tags): + tagged.add(task_file.stem) + + text = self.README.read_text(encoding="utf-8") + assert "Members:" in text, "the smoke Members list is gone; update this test" + block = text.split("Members:", 1)[1].split("\n\n", 1)[0] + listed = set(re.findall(r"`([^`]+)`", block)) + + assert not tagged - listed, ( + f"tasks/README.md's smoke Members list omits {sorted(tagged - listed)}. " + "The README is the map for this directory; add the task or drop the tag." + ) + + +class TestRecordCliProbeIntegrity: + """The probe's detectors must stay wired to the stub they detect. + + Mirrors TestAntiCheatProbeIntegrity: a probe whose detector drifts from the + thing it detects reports a pass forever, including after a real regression. + These turn the task's own prose invariants into tests. + + Reads the task through `TaskDefinition` rather than raw YAML, so every default + (a rule's `exit_code`, an omitted `stdout`) comes from the models instead of + being hand-copied here -- a copied default stops matching the shim silently. + """ + + TASK = Path("tasks/record_cli_responses.yaml") + LOG = "cli_mocks/calls.jsonl" + + def _task(self) -> TaskDefinition: + if not self.TASK.exists(): + pytest.skip("probe task not present") + return TaskDefinition(**yaml.safe_load(self.TASK.read_text(encoding="utf-8"))) + + def _entry(self, task: TaskDefinition): + entries = task.sandbox.record_cli + assert len(entries) == 1, ( + f"this probe's tests assume exactly one record_cli entry, found {len(entries)}. " + "Adding a second stubbed tool means teaching them which entry to read." + ) + return entries[0] + + def test_dispatch_is_proved_against_a_real_shim_log(self): + """The authoritative detector, checked against a log the SHIM actually wrote. + + `cli_called` matches argv only, so it passes whether a rule answered or the + entry fallback did. `captured.txt` is transcribable: this YAML is serialised + to /work/input and mounted at /work/task_dir, both readable. And a codegen + regression that renders `RULES = []` raises nothing, so neither `rule_error` + nor `sidecar_error` is booked. Only the `"rule": N` key catches that. + + So this test does not compare the YAML against itself. It generates the + task's own record_cli entry, RUNS each stubbed command, and requires the + criteria's regexes to match the resulting real log lines -- because the + needle's exact spelling (`"rule": 0`, with the space) belongs to + `json.dumps`'s default separators in `invocation_log.record`, not to this + test. Switching the shim to compact separators would otherwise leave this + green while the blocking CI probe failed. + """ + task = self._task() + entry = self._entry(task) + patterns = [ + c.pattern + for c in task.success_criteria + if c.type == "file_matches_regex" and c.path == self.LOG and c.must_match + ] + assert patterns, ( + f"no must-match file_matches_regex criterion reads {self.LOG!r}. Without it this probe " + "reports SUCCESS when per-invocation dispatch is dead but the agent still ran the commands." + ) + + config = SandboxConfig(driver="tempdir", python=None, record_cli=[entry]) + sandbox = Sandbox(config, task_id="probe_integrity_log") + try: + sandbox_dir = sandbox.setup() + for rule in entry.responses: + (tokens,) = rule.when.match_spec["verb_spellings"] + subprocess.run( + [sys.executable, str(sandbox_dir / RECORD_CLI_DIR / entry.tool), *tokens], + capture_output=True, + text=True, + encoding="utf-8", + check=False, + ) + log_lines = (sandbox_dir / RECORD_CLI_LOG).read_text(encoding="utf-8").splitlines() + finally: + sandbox.cleanup(preserve=False) + + assert len(log_lines) == len(entry.responses), "the stub did not record every invocation" + # Every rule must be pinned to the line IT produced. Requiring the indices + # to appear merely somewhere would accept a regression that swapped them. + for index, line in enumerate(log_lines): + assert any(re.search(pattern, line) for pattern in patterns), ( + f"no log criterion matches the real record for responses[{index}]:\n {line}\n" + "The probe's regexes have drifted from what the shim writes, so the blocking CI " + "task would fail while this test stayed green." + ) + assert f'"rule": {index}' in line, ( + f"responses[{index}] did not answer its own invocation; the shim recorded: {line}" + ) + + def test_the_expected_strings_come_from_the_stub_not_the_prompt(self): + """Keeps the response strings out of the prompt. + + This does NOT make them unobtainable -- the task YAML is readable in the + sandbox, which is why the log criterion above is the authoritative detector. + It removes the cheapest transcription path: a prompt that named the strings + would let an agent satisfy `captured.txt` without running anything at all. + """ + task = self._task() + served = {rule.stdout.strip() for rule in self._entry(task).responses} + wanted = [ + needle + for c in task.success_criteria + if c.type == "file_contains" and c.path == "captured.txt" + for needle in c.includes + ] + + assert wanted, "the probe no longer checks what the agent captured" + for needle in wanted: + assert any(needle in text for text in served), ( + f"captured.txt wants {needle!r} but no record_cli response serves it" + ) + assert needle not in task.initial_prompt, ( + f"{needle!r} appears in initial_prompt, so the criterion is satisfiable by " + "transcription without the agent running the tool at all" + ) + + def test_every_asserted_verb_has_a_matching_response_rule(self): + """Pins each cli_called detector to a rule, the way the anti-cheat probe pins + its regex to its canary.""" + task = self._task() + rule_verbs = { + tuple(tokens) for rule in self._entry(task).responses for tokens in rule.when.match_spec["verb_spellings"] + } + asserted = [c for c in task.success_criteria if c.type == "cli_called"] + + assert asserted, "the probe no longer asserts any invocation" + for criterion in asserted: + for tokens in criterion.verb_spellings: + assert tuple(tokens) in rule_verbs, ( + f"cli_called asserts verb {' '.join(tokens)!r}, which no response rule serves — " + "the probe would pass on the entry fallback and prove nothing about dispatch" + ) + + def test_the_fallback_differs_from_every_rule(self): + """ "Every rule served the entry default" must be distinguishable from a pass. + + Defaults come from the models, not from literals here: `CliResponse.exit_code` + defaults to 0 while `RecordedCli.exit_code` defaults to 1, and neither rule in + the task sets one, so a hand-copied default that drifted would leave this + comparing a tuple the shim never serves. + """ + entry = self._entry(self._task()) + fallback = (entry.exit_code, entry.stdout, entry.stderr) + + for index, rule in enumerate(entry.responses): + served = (rule.exit_code, rule.stdout, rule.stderr) + assert served != fallback, ( + f"responses[{index}] is byte-identical to the entry fallback, so this probe cannot " + "tell per-rule dispatch from no dispatch at all" + ) + + class TestAntiCheatProbeIntegrity: """The probe's leak detector must stay wired to its own canary."""