Skip to content

fix(command-executed): match patterns against shell-normalized commands - #77

Merged
rockymadden merged 11 commits into
mainfrom
fix/command-executed-shell-normalize
Aug 5, 2026
Merged

fix(command-executed): match patterns against shell-normalized commands#77
rockymadden merged 11 commits into
mainfrom
fix/command-executed-shell-normalize

Conversation

@rockymadden

@rockymadden rockymadden commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Problem

The command_executed criterion runs the author's command_pattern regex only against the raw bash -lc "…" wrapper string. So whichever way the agent happened to quote an argument — bare, "double", 'single', \"escaped\" — leaks into every pattern, and authors have to hand-model shell escaping. They do it inconsistently: 173 of 1,557 command_pattern criteria encode quote-tolerance, across four different idioms (["\']?, ["\x27]?, ["\047]?, \\?"?).

Surfaced by the skill-flow-paginated-reference-lookup eval. The task pattern used \\?"? (optional backslash + optional double quote):

uip\s+is\s+resources\s+run\s+list\s+\\?"?uipath-salesforce-slack\\?"?\s+\\?"?curated_channels

The agent paginated correctly but single-quoted the resource arg:

/bin/bash -lc "uip is resources run list uipath-salesforce-slack 'curated_channels?types=public_channel,private_channel' --output json"

The ' isn't covered by \\?"?, so the gating "paginated more than once" criterion scored 0.0 — a false negative — even though the sibling nextPage= criterion matched the very same calls (3/1). Net: a fully correct run scored FAILURE / 0.76.

Fix

Normalize before matching. _normalize_shell() unwraps a bash/sh -c wrapper and resolves shell quoting via shlex; the matcher then searches both the raw text and the normalized form (either hit counts). Applied to include and exclude patterns, in the shared _matching_commands helper so the score and the live early-stop trigger never disagree.

Properties:

  • Additive — nothing that matched the raw text before can stop matching.
  • Operators survive&&, |, > remain as tokens, so patterns referencing them keep working.
  • Safe fallback — unparseable input (unbalanced quotes, heredocs) → None → raw text only, no crash.
  • ReDoS bound preserved — both haystacks length-capped.
  • No task-YAML changes needed — the whole quote-tolerance class is fixed at the matcher; the 173 hand-rolled idioms become unnecessary going forward.

Verification

  • New regression tests in test_command_executed.py: the unchanged YAML pattern now counts the single-quoted calls 2/2; \"-escaped form still matches (backward compat); && patterns survive; unbalanced quotes fall back without crashing; a max_count: 0 negative assertion can't be dodged by quoting. _normalize_shell unit tests included.
  • tests/test_command_executed.py: 35/35 pass. Full non-live suite: 3,714 pass, 7 skipped (one pre-existing live-SDK timeout unrelated to this change). ruff check/format + pyright clean; custom architectural lint 158/158.
  • End-to-end: replaying the four real recorded commands from the failed run through the new matcher with the unchanged task pattern → 4/4 match (was 0).

🤖 Generated with Claude Code


Correction & re-baseline note (post-review)

The Fix section above claims this change is "additive — nothing that matched before can stop matching." That is not accurate and should not be relied on. The normalized haystack also feeds exclude_pattern and the max_count gate, so normalization can newly satisfy an exclusion or trip a max_count: 0 gate — a command that counted on the raw text alone can now stop counting. (Verified: /bin/bash -lc "'uip' or users list" with command_pattern: uip\s+or\s+users\s+list, min_count: 0, max_count: 0 scored 1.0 before and 0.0 after.) command_pattern matches remain monotonic (can only be gained); the criterion's overall verdict is bidirectional.

Blast radius / re-baseline: the in-repo tasks/ tree has a single command_pattern. The real consumers are the out-of-repo suites (coder-eval-uipath) whose ~173 hand-rolled quote-tolerant patterns become unnecessary. Those suites should re-baseline — scores can move in either direction on unedited YAML. Authoritative P/R/F1 for early-stop suites still comes from a stop_early: false run.

Follow-up commits (the review's Top 5 Priority Actions)

  1. d26fdad9 — narrow the command param to str before shell-normalizing. A Codex sub-agent argv-list command reached shlex.split and raised AttributeError: 'list' object has no attribute 'read', zeroing an otherwise-passing criterion.
  2. 26c40624 — keep the whole argv-joined payload in the unwrap. Codex rollout recovery joins argv without re-quoting, so bash -lc uip is resources … collapsed to just uip (fix was a silent no-op there; a degenerate one-word haystack could falsely match an anchored pattern).
  3. 4f578014 — recognize shell wrappers by predicate, not an enumerated allowlist. zsh -lc (Codex on macOS) and -ic were never unwrapped, so identical agent behavior scored differently by host shell.
  4. 7d42c5fa — make _match_haystacks total (explicit is_shell, decided once at the extraction site), normalize the already-truncated window so both haystacks share one 2000-char slice (retires _MAX_NORMALIZE_LEN), and memoize _normalize_shell for the early-stop hot path (was re-lexed O(n²) per run). Module now at 100% statement + branch coverage.
  5. 96d26a40 — document the raw-OR-normalized contract (both Field descriptions + a "Shell normalization" paragraph in TASK_DEFINITION_GUIDE.md) and correct the _normalize_shell docstring's false invariants.

CE030 deferral (d8a77ea9). Action 5 also attempted to extend the CE030 doc/schema-parity lint to the SuccessCriterion union (so a future criterion field can't ship undocumented), but it was reverted: CI installs --extra uipath, and in that environment coder_eval.models.criteria acquires a CliCalledCriterion (fields log/positional) that is absent from a plain checkout and indistinguishable from an in-tree criterion by every runtime and source signal available to the lint (it spoofs __module__, is setattr onto the module, and appears in the imported module's union literal). CE030 stays scoped to the four top-level models; the union extension is parked in .claude/harness-candidates.md with the full diagnosis. The contract change this PR makes is documented regardless (Field descriptions + guide).

Full non-live suite after the follow-ups: 3,729 pass, 7 skipped (the one remaining failure is test_agent_judge_integration_real_sdk, a real-SDK integration test that times out offline — pre-existing, unrelated). ruff/pyright clean; custom architectural lint 158/158.

`command_pattern` regexes were run only against the raw `bash -lc "..."`
wrapper string, so whichever way the agent happened to quote an argument
(bare, "double", 'single', \"escaped\") leaked into the pattern. Authors
hand-model that escaping and get it subtly wrong: ~173/1557 criteria encode
quote-tolerance, in four inconsistent idioms. skill-flow-paginated-reference-lookup
used `\\?"?` (double-quote only); the agent single-quoted the resource arg,
so a correct multi-call pagination loop scored 0.0 on a gating criterion —
a false negative — while the sibling `nextPage=` criterion matched the same calls.

Normalize before matching: unwrap the `bash`/`sh -c` wrapper and resolve shell
quoting via shlex, then match against BOTH the raw text and the normalized
form (either hit counts). This is additive — nothing that matched before can
stop matching — and lands in the shared `_matching_commands` helper so the
score and the live early-stop trigger stay consistent. Shell operators
(`&&`, `|`, `>`) survive as tokens; unparseable input (unbalanced quotes,
heredocs) falls back to the raw text. Fixes the whole class with no task-YAML
changes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Claude finished @rockymadden's task in 1m 15s —— View job


Code Review in Progress

Todo List:

  • Read .github/code_review.md for review standards
  • Read the full diff (git diff origin/main...HEAD)
  • Read complete context of changed files
  • Analyze correctness and edge cases
  • Check cross-file consistency
  • Identify what might be missing
  • Provide design-level scrutiny
  • Post comprehensive review

Starting review now...

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Scope: pr:77 (2 files) axis:1,2,3,4,5,6,7,8 · branch fix/command-executed-shell-normalize · b2f5401 · 2026-08-04T14:28Z · workflow variant

Change class: complex — changes how every command_executed criterion matches, adding a second shlex-normalized haystack for both command_pattern and exclude_pattern, so existing task-YAML scoring semantics shift

Architecture, security, and error handling are in excellent shape (9.8-10) and the PR's intent — stop losing command_executed matches to shell quoting — is right, but the whole risk surface sits in one ~60-line helper in src/coder_eval/criteria/command_executed.py whose unvalidated Any-typed input, incomplete shell allowlist, truncated-argv unwrap, and undocumented widening can each flip a task's score between 1.0 and 0.0 for byte-identical agent output; bottom line: merge only after the extraction site is type-narrowed, the unwrap handles zsh and argv-joined forms, and the author-facing contract (plus the commit message's disproven "nothing that matched before can stop matching" claim) is corrected.

Summary

Axis Score 🔴 🟠 🟡 🔵 Top Issue
1. Code Quality & Style 9.4 / 10 0 0 1 1 Shell-unwrap allowlists _SHELL_WRAPPERS/_SHELL_CMD_FLAGS (command_executed.py:28-29) are incomplete/inconsistent — zsh and -ic missing while -lic is listed — so Codex-on-macOS zsh -lc telemetry is never unwrapped and identical agent behavior scores differently by host shell
2. Type Safety 8.4 / 10 0 1 1 1 cmd_text: str annotation is unenforced — Any from CommandTelemetry.parameters reaches shlex.split/slicing and flips a score from 1.0 to 0.0
3. Test Health 9.5 / 10 0 0 1 0 Every new normalization degradation/edge branch is unexercised (inner ValueError, empty-token, non-wrapper break, _MAX_NORMALIZE_LEN skip, normalized-haystack re-truncation) — they are 100% of the module's uncovered lines
4. Security 9.9 / 10 0 0 0 1 shlex.split is run on up to 20 000 chars of agent-controlled command text inside the per-tool-call early-stop callback, a ~4400x per-command CPU amplification over the previous regex-only path
5. Architecture & Design 10 / 10 0 0 0 0
6. Error Handling & Resilience 9.8 / 10 0 0 0 2 _normalize_shell's None return conflates parse error with benign empty, and the inner except ValueError discards the already-successful outer normalization
7. API Surface & Maintainability 8.9 / 10 0 1 0 1 Silent semantics change to the documented command_pattern/exclude_pattern task-author contract: unedited YAML re-scores in both directions, yet the contract is documented only in a private helper docstring — guide and Field descriptions unchanged, no opt-out
8. Evaluation Harness Quality 9 / 10 0 1 0 0 bash -lc unwrap keeps only tokens[i+1], so an argv-joined (unquoted) payload collapses to its first word — the normalization is a no-op on Codex's sub-agent rollout-recovery telemetry and can create a degenerate one-word haystack

Overall Score: 9.4 / 10 · Weakest Axis: Type Safety at 8.4 / 10
Totals: 🔴 0 · 🟠 3 · 🟡 3 · 🔵 6 across 8 axes.

Blockers

  1. [Axis 2] cmd_text: str annotation is unenforced — Any from CommandTelemetry.parameters reaches shlex.split/slicing and flips a score from 1.0 to 0.0 (src/coder_eval/criteria/command_executed.py:127) — Line 127 cmd_text = cmd.parameters["command"] reads a dict[str, Any] value (models/telemetry.py:367 parameters: dict[str, Any]) behind only a truthiness guard (line 126 cmd.parameters.get("command")), so Any silently satisfies the new str parameters on line 73 def _match_haystacks(cmd: "CommandTelemetry", cmd_text: str) -> list[str] and line 32 def _normalize_shell(cmd_text: str) -> str | None. pyright reports nothing because Any is assignable to str. A non-str command is reachable in production: codex_agent.py:1919-1923 returns parsed function_call.arguments verbatim (Codex's shell tool passes command as an argv ARRAY, and if "cmd" in parsed ... parsed["command"] = parsed["cmd"] copies a list unchanged) while _subagent_tool_name maps shell/exec_command/local_shell to "Bash" (_ROLLOUT_FN_NAMES, codex_agent.py:152-156), and both land in CommandTelemetry(tool_name=call["tool_name"], parameters=call["parameters"], ...) at codex_agent.py:1970-1977. Because line 134 haystacks = _match_haystacks(cmd, cmd_text) now runs BEFORE the pattern checks (it used to be if len(cmd_text) > ... only, and .search() was skipped entirely when no pattern was configured), a pattern-less criterion regresses. Verified with identical telemetry, CommandExecutedCriterion(tool_name="Bash", min_count=1) and parameters={'command': ['bash','-lc','ls']}: base main scores 1.0/error=None; PR HEAD scores 0.0 with AttributeError: 'list' object has no attribute 'read' (raised inside shlex.split at line 50, which only guards except ValueError: on line 51). One such record poisons the whole criterion, since the exception aborts _matching_commands for every command in the trajectory. Fix: narrow at the extraction site the way the sibling criterion already does — criteria/skill_triggered.py:67-71 uses if isinstance(skill, str) and skill: / if isinstance(value, str): — e.g. raw = cmd.parameters.get("command"); cmd_text = raw if isinstance(raw, str) and raw else json.dumps(cmd.parameters), so the str annotations on lines 32/73 become true. Consider a CExxx lint rule: a cmd.parameters[...]/.get(...) value passed to a str-typed parameter or a str-only stdlib call without an isinstance(..., str) narrow.
  2. [Axis 7] Silent semantics change to the documented command_pattern/exclude_pattern task-author contract: unedited YAML re-scores in both directions, yet the contract is documented only in a private helper docstring — guide and Field descriptions unchanged, no opt-out (src/coder_eval/criteria/command_executed.py:137) — FAILURE SCENARIO (verified by running the identical CommandTelemetry through both revisions): command /bin/bash -lc "'uip' or users list", criterion command_pattern: uip\s+or\s+users\s+list, min_count: 0, max_count: 0main scores 1.0, pr-77 scores 0.0 (the PR's own tests/test_command_executed.py:878 test_negative_assertion_not_dodged_by_quoting asserts the 0.0). Identical agent output, opposite verdict, on YAML nobody edited. The reverse direction is also live (tests/test_command_executed.py:826 shows a previously-0.0 pattern now scoring 1.0), and exclude_pattern gains the same widening, so a command that formerly counted can now be silently excluded. This repo's tasks/ has exactly one command_pattern (tasks/agents/subagent_bash_long_input.yaml:74), so the real consumers are the out-of-repo suites (coder-eval-uipath) whose authors have only the field description and the guide to read — neither mentions normalization, and there is no field to opt out.

EVIDENCE: the PR widens matching from one haystack to two — src/coder_eval/criteria/command_executed.py:137 if pattern is not None and not any(pattern.search(h) for h in haystacks): and :142 if exclude_re is not None and any(exclude_re.search(h) for h in haystacks): — but the documented contract is untouched. models/criteria.py:515-517 still reads command_pattern: str | None = Field(default=None, description="Regex to match command parameters. None = any command."); models/criteria.py:537-542 still reads "Regex that must NOT match. Commands matching both command_pattern and exclude_pattern are skipped."; docs/TASK_DEFINITION_GUIDE.md:754 still reads command_pattern: "curl.*wttr\\.in" # Regex to match command parameters (null = any). Per this repo's DRY principle the field description IS the documentation, and CE030 does not track criterion models, so nothing mechanically gates the drift.

FIX: (a) state in both command_pattern and exclude_pattern descriptions that a Bash command is matched against the raw text OR its shlex-resolved, wrapper-stripped form (whichever hits), and that exclude_pattern therefore also excludes quote-obfuscated forms; (b) add a short "shell normalization" paragraph to the command_executed section of docs/TASK_DEFINITION_GUIDE.md (after line 758) with the bash -lc "... 'arg' ..." example; (c) note the score-direction change in the PR description / CHANGELOG so cross-repo suite owners can re-baseline; (d) propose extending the CE030 doc/schema-parity rule (or a new CEnnn) to SuccessCriterion models so the next semantics change to a criterion field cannot ship undocumented.
3. [Axis 8] bash -lc unwrap keeps only tokens[i+1], so an argv-joined (unquoted) payload collapses to its first word — the normalization is a no-op on Codex's sub-agent rollout-recovery telemetry and can create a degenerate one-word haystack (src/coder_eval/criteria/command_executed.py:66) — Lines 61-67 take the script to be a single token — inner = shlex.split(tokens[i + 1], posix=True)tokens = inner — and discard everything after it. Codex records shell telemetry by joining argv WITHOUT re-quoting (src/coder_eval/agents/codex_agent.py:1928-1929: if isinstance(command, list): return {"command": " ".join(str(c) for c in command)}), i.e. exactly bash -lc <script words…>. Verified against the PR's own fixture payload:
_normalize_shell("bash -lc uip is resources run list uipath-salesforce-slack 'curated_channels?types=x' --output json")'uip' (haystacks become [<raw>, 'uip']), and _normalize_shell("bash -c echo hi there")'echo'.

Two consequences: (1) on the Codex path — the agent whose telemetry actually carries the bash -lc wrapper the docstring cites at line 37 — the fix is a silent no-op, so the false-negative class the PR set out to eliminate persists; (2) the degenerate one-word haystack can newly MATCH: with command_pattern: ^git\b and command bash -lc git push --force origin main, raw does not match but the normalized 'git' does (verified match_raw=False, any(haystacks)=True) — under min_count: 0, max_count: 0 that flips a passing negative assertion to 0.0. Fix: rejoin the remainder (inner = shlex.split(tokens[i + 1]) or tokens[i + 1:], or fall back to tokens[i + 1:] when the payload is multi-token) and add tests for the argv-joined shape; note that coverage's missed branches 65->67 / lines 63-64,68-69 sit precisely in this unwrap loop.

Non-blocking, but please consider before merge

  1. [Axis 1] Shell-unwrap allowlists _SHELL_WRAPPERS/_SHELL_CMD_FLAGS (command_executed.py:28-29) are incomplete/inconsistent — zsh and -ic missing while -lic is listed — so Codex-on-macOS zsh -lc telemetry is never unwrapped and identical agent behavior scores differently by host shell (src/coder_eval/criteria/command_executed.py:28) — Lines 28-29 read _SHELL_WRAPPERS = {"bash", "sh"} / _SHELL_CMD_FLAGS = {"-c", "-lc", "-lic"}. Verified against PR HEAD: _normalize_shell('zsh -lc "uip is resources run list slack curated_channels"') returns 'zsh -lc uip is resources run list slack curated_channels' (wrapper NOT stripped) and _normalize_shell('bash -ic "uip is resources run list slack"') returns 'bash -ic uip is resources run list slack' — i.e. for these forms the fix silently reverts to the pre-PR false-negative behavior the PR exists to remove, and the failure mode is invisible (a criterion just scores 0.0 again). zsh is not hypothetical: this repo's own Codex agent documents it at src/coder_eval/agents/codex_agent.py:1130 — "Codex issues every shell command through the user's default shell as a login shell: bash -lc on Linux, zsh -lc on macOS". The set is also self-inconsistent: the exotic -lic is listed while the more common -ic is not. A denylist-of-known-strings is the wrong mechanism for an open set; replace it with a generic predicate — basename matching a shell ({"bash","sh","zsh","dash","ksh"} at minimum, or name.endswith("sh")) and "first --prefixed token whose characters are all shell option letters and which contains c" instead of an enumerated flag set (this also collapses the special cases, since bash -l -c already works via the loop at lines 58-69). Add one TestNormalizeShell case per wrapper form so the set can't silently rot.
  2. [Axis 2] _match_haystacks re-derives shell-ness from tool_name == "Bash" alone (line 84), diverging from the caller's tool_name == "Bash" and parameters.get("command") guard, so a Bash record with a missing/empty command gets its JSON-serialized params shlex-normalized — contradicting the helper's docstring, able to flip a score via exclude_pattern, and untested (src/coder_eval/criteria/command_executed.py:84) — Line 84 if cmd.tool_name == "Bash" and 0 < len(cmd_text) <= _MAX_NORMALIZE_LEN: reconstructs the caller's decision from the over-broad cmd: "CommandTelemetry" parameter (line 73) instead of being told what cmd_text actually is. The caller's guard is stricter — line 126 if cmd.tool_name == "Bash" and cmd.parameters.get("command"): — so a Bash record whose command is absent/empty falls to line 129 cmd_text = json.dumps(cmd.parameters) and is STILL shell-normalized, contradicting the function's own docstring (lines 80-81: 'Non-Bash tools serialize params to JSON, where shell tokenization is meaningless, so they are never normalized'). shlex strips the JSON quotes, adding a second haystack that can newly satisfy exclude_pattern at line 142 and drop a command from the count. Verified with identical telemetry, parameters={'description': 'run the pytest suite'} (Bash, no command key) and CommandExecutedCriterion(tool_name='Bash', exclude_pattern=r'description:\s+run', min_count=1): base main scores 1.0, PR HEAD scores 0.0; _match_haystacks returns ['{"description": "run the pytest suite"}', '{description: run the pytest suite}']. An empty command is reachable — codex_agent.py:2051 command = getattr(command_item, "command", "") defaults to "", and codex_agent.py:1931 dict(action) can omit command entirely. Fix: narrow the signature so the flag cannot drift from the caller, e.g. def _match_haystacks(cmd_text: str, *, is_shell: bool) -> list[str] called as _match_haystacks(cmd_text, is_shell=is_shell) where is_shell is the same boolean computed once at line 126. That also makes the helper total and unit-testable without constructing a CommandTelemetry.
  3. [Axis 3] Every new normalization degradation/edge branch is unexercised (inner ValueError, empty-token, non-wrapper break, _MAX_NORMALIZE_LEN skip, normalized-haystack re-truncation) — they are 100% of the module's uncovered lines (tests/test_command_executed.py:782) — TestNormalizeShell covers only the outer parse failure (line 782: assert _normalize_shell("echo 'unterminated") is None). Per-module coverage across the full suite is 93.57% with Missing: 54, 58->70, 63-64, 65->67, 68-69 — i.e. 100% of the module's uncovered lines/partials are the new helper's own branches: if not tokens: return None (54), the wrapper scan loop exhausting (58->70), the INNER shlex.split(tokens[i + 1], posix=True) raising (63-64), if inner: false (65->67), and break # first positional before any -c: not a command wrapper (68-69). These are one-line unit tests since the helper is already imported at line 5. One of them returns visibly odd output that no test would currently catch — I ran _normalize_shell('bash -lc ""') at PR HEAD and got 'bash -lc ' (wrapper left in place, trailing space) rather than None/''. Add cases for: ""/whitespace-only input, """bash -lc 'echo \"unterminated'""" (inner ValueError → None), 'bash -lc ""', and "bash script.sh -c foo" (non-wrapper positional → returned verbatim).

Nits

  1. [Axis 1] _MAX_NORMALIZE_LEN is an admitted-unnecessary guard derived from an unrelated ReDoS constant, and its companion 0 < len(cmd_text) check is dead (src/coder_eval/criteria/command_executed.py:25) — Line 25 declares _MAX_NORMALIZE_LEN = 10 * _MAX_PATTERN_SEARCH_LEN with the comment "shlex is linear so this is only a worst-case guard; real telemetry commands are far smaller" — machinery the code itself says it does not need, and its value is bound to a constant (_MAX_PATTERN_SEARCH_LEN, the regex ReDoS cap on line 21) whose meaning is unrelated, so tuning one silently moves the other. Its only effect is that commands over 20 000 chars are not unwrapped at all, quietly restoring the bug this PR fixes for long commands. In the same guard at line 84, 0 < len(cmd_text) is redundant: verified at PR HEAD that _normalize_shell("") and _normalize_shell(" ") both return None, which line 86 already handles. Simplify to if cmd.tool_name == "Bash": (dropping both), or, if a size cap is genuinely wanted, give it a standalone literal plus a rationale that isn't "shlex is linear".
  2. [Axis 2] New module-level membership constants are mutable set literals, diverging from the repo's frozenset convention (src/coder_eval/criteria/command_executed.py:28) — Lines 28-29 declare _SHELL_WRAPPERS = {"bash", "sh"} and _SHELL_CMD_FLAGS = {"-c", "-lc", "-lic"} as mutable, unannotated set[str]. Every comparable module-level membership constant in src/ is an immutable, often annotated frozenset — e.g. agents/codex_agent.py:163 _ROLLOUT_TOOL_CALL_TYPES = frozenset({...}), models/results.py:240 _CLASSIFICATION_CRITERION_TYPES = frozenset({"classification_match", "skill_triggered"}), isolation/docker_runner.py:66 _LOOPBACK_HOSTS = frozenset({"localhost", "127.0.0.1", "::1"}), agents/antigravity_agent.py:127 _RESULT_ARG_KEYS: frozenset[str] = frozenset({...}). Change to _SHELL_WRAPPERS: frozenset[str] = frozenset({"bash", "sh"}) and _SHELL_CMD_FLAGS: frozenset[str] = frozenset({"-c", "-lc", "-lic"}) so the constants are type-annotated and cannot be mutated by an importer.
  3. [Axis 4] shlex.split is run on up to 20 000 chars of agent-controlled command text inside the per-tool-call early-stop callback, a ~4400x per-command CPU amplification over the previous regex-only path (src/coder_eval/criteria/command_executed.py:84) — The offending gate admits ten times the previous ReDoS bound into a pure-Python character-at-a-time lexer:
25: _MAX_NORMALIZE_LEN = 10 * _MAX_PATTERN_SEARCH_LEN
...
84:     if cmd.tool_name == "Bash" and 0 < len(cmd_text) <= _MAX_NORMALIZE_LEN:
85:         normalized = _normalize_shell(cmd_text)

Good news first, since these were the stated concerns and both check out: shlex.split IS linear on adversarial input (measured on 20 000-char payloads: 20 000 nested/alternating quotes 3.2-3.8 ms, 10 000 escapes 6.4 ms, one 20 000-char token 6.4 ms) so there is no super-linear parse bomb, and the regex haystack bound is preserved exactly (both entries are sliced [:_MAX_PATTERN_SEARCH_LEN] on lines 83 and 87, so the change is a 2x, not an unbounded, regex-input increase).

The residual issue is the per-command constant. Measured through the real code path (CommandExecutedChecker._matching_commands, 200 Bash commands of exactly 20 000 chars, pattern curl):

main : 200-command scan  0.14 ms  -> 0.0007 ms/command
pr-77: 200-command scan 620.60 ms -> 3.10 ms/command   (~4400x)

That cost lands in EarlyStopWatcher._evaluate (src/coder_eval/orchestration/early_stop.py:373), which calls live_verdict -> _matching_commands over the WHOLE accumulated trajectory on every ToolStartEvent AND every resolved ToolEndEvent (on_event, lines 291-341) — i.e. ~2 full re-scans per tool call. Simulating that quadratic accumulation for 200 such commands gives ~125 s of synchronous CPU burned inside a StreamCallback on the agent's asyncio loop, versus well under a second on main. An evaluated agent that emits large heredoc/script commands (not even maliciously) can therefore stall a stop_early run. Note also that the bound is off by one against its own intent: a 20 001-char command skips normalization entirely, so pattern semantics silently change at the cap.

Fix (cheap, no semantic change): memoize the normalized form per CommandTelemetry (e.g. cache keyed on cmd.tool_id/sequence_number, or compute it once when telemetry is captured) so each command is lexed at most once per run instead of once per criterion per tool call; and consider lowering _MAX_NORMALIZE_LEN toward _MAX_PATTERN_SEARCH_LEN, since anything beyond the first 2000 chars of the normalized string is discarded by the slice on line 87 anyway. CVSS:3.1/AV:L/AC:L/PR:L/UI:N/S:U/C:N/I:N/A:L
4. [Axis 6] _normalize_shell's None return conflates parse error with benign empty, and the inner except ValueError discards the already-successful outer normalization (src/coder_eval/criteria/command_executed.py:63) — At lines 61-64 the wrapper-unwrap does try: inner = shlex.split(tokens[i + 1], posix=True) / except ValueError: return None. By this point the OUTER split has already succeeded and tokens holds a valid, quote-resolved form; return None throws it away and degrades all the way back to the raw text. Verified in the PR venv: _normalize_shell('/bin/bash -lc "uip is resources run list \'chan\' && echo don\'t"') -> None, even though /bin/bash -lc uip is resources run list 'chan' && echo don't (outer \" escapes resolved) was in hand and is a strictly better haystack than the raw string. Return " ".join(tokens) instead of None there. Separately, None is overloaded across three distinct outcomes — outer parse error (line 52), benign empty input (if not tokens: return None, lines 53-54), and inner parse error (line 64) — so the caller at line 86 (if normalized is not None and normalized != cmd_text:) cannot tell "nothing to normalize" from "normalization failed", which is exactly the distinction finding #1's log needs.
5. [Axis 6] Normalization docstring invariants are provably false (heredocs parse fine; normalization CAN remove a match via exclude_pattern) and the same rationale is restated in four places (src/coder_eval/criteria/command_executed.py:79) — (a) Lines 45-47 claim "Returns None when the text can't be parsed (unbalanced quotes, heredocs)". Heredocs parse fine — shlex.split('bash -lc "cat <<EOF\\nhello\\nEOF"', posix=True) returns ['bash', '-lc', 'cat <<EOF\\nhello\\nEOF'], and _normalize_shell('bash -lc "python - <<PY\\nprint(\'a\')\\nPY"') returns 'python - <<PY print(a) PY', not None. Drop "heredocs" (the real trigger is an odd quote count) and note that newlines are collapsed to single spaces. (b) Lines 78-80 state: "Matching is "either" — a pattern hits the command if it matches ANY haystack — so normalization only ever repairs a missed match, never removes one." That invariant holds for command_pattern but is false for the function as used: line 142 feeds the same haystacks to any(exclude_re.search(h) for h in haystacks), so an added haystack can newly EXCLUDE a command that previously counted, and line 137's any(...) can newly trip a max_count: 0 gate — the PR's own test_negative_assertion_not_dodged_by_quoting (tests/test_command_executed.py:878) asserts result.score == 0.0 on input that scored 1.0 before this change. This is the one sentence a future reader would cite to conclude the change is risk-free, so it must say "can newly satisfy an exclusion or trip a max_count cap" instead. (No task YAML currently sets exclude_patterngrep -rn exclude_pattern tasks/ returns nothing — which is why this is Low rather than higher.)
6. [Axis 7] Match window is asymmetric between the two haystacks: patterns can now match command text beyond the documented 2000-char cap, contradicting the helper's own "both are length-capped" claim (src/coder_eval/criteria/command_executed.py:83) — FAILURE SCENARIO (verified on both revisions): a 2426-char Bash command /bin/bash -lc "'a' 'a' ... TARGET_CMD" whose TARGET_CMD token starts at offset 2415 (past _MAX_PATTERN_SEARCH_LEN = 2000), criterion command_pattern: TARGET_CMD, min_count: 1main scores 0.0 (target lies outside the truncated haystack, as intended), pr-77 scores 1.0 (quote-stripping compresses the normalized form so the target lands at ~offset 1215). A task relying on the 2000-char cap therefore changes verdict for identical agent output, and no test in tests/test_command_executed.py pins either window.

EVIDENCE: :83 haystacks = [cmd_text[:_MAX_PATTERN_SEARCH_LEN]] truncates the raw text at 2000 chars, while :84-87 runs _normalize_shell over the full text (up to _MAX_NORMALIZE_LEN = 10 * _MAX_PATTERN_SEARCH_LEN, :25) and only then truncates: haystacks.append(normalized[:_MAX_PATTERN_SEARCH_LEN]). Because normalization strips quotes and the wrapper, content that sat past char 2000 slides inside the normalized window. The docstring at :77-78 ("Both are length-capped to preserve the ReDoS bound") is true for ReDoS but hides this visibility change.

FIX: either normalize cmd_text[:_MAX_PATTERN_SEARCH_LEN] so both haystacks describe the same window, or amend the docstring (and the command_pattern field description) to state that the normalized haystack may expose text beyond the raw cap.

What's Missing

Parallel paths:

  • 🟡 criteria/skill_triggered.py is the other criterion that pattern-scans the same Bash command telemetry (_SKILL_PATH_RE over cmd.parameters string values) and still hand-models shell/JSON escaping (skills[\\/]+ to survive doubled backslashes) — exactly the class this PR eliminated for command_executed — so the harness now holds two different notions of "the command the agent ran"; the new _normalize_shell is private to one module instead of a shared helper (e.g. criteria/_shell_normalize.py, alongside the existing criteria/_classification_aggregate.py) reusable by skill_triggered, evaluation/summaries.py::summarize_commands, and judge_context.py:384 (which still hand the LLM/agent judge the raw wrapped text a criterion may now match only in normalized form). (trigger: src/coder_eval/criteria/command_executed.py)
  • 🟡 The wrapper/flag knowledge was written from scratch in _SHELL_WRAPPERS/_SHELL_CMD_FLAGS instead of reusing what the repo already knows about its own agents — agents/codex_agent.py:1130 documents bash -lc on Linux and zsh -lc on macOS, and _ROLLOUT_FN_NAMES/_TOOL_NAME map shell/local_shell/exec_command to tool_name="Bash" — so the shell-wrapper set in the criterion and the shell-wrapper reality in the agent layer have already diverged. (trigger: src/coder_eval/criteria/command_executed.py) (restates: Axis 1: Shell-unwrap allowlists _SHELL_WRAPPERS/_SHELL_CMD_FLAGS are incomplete/inconsistent (zsh, -ic missing))

Tests:

  • 🟠 No test covers the new exclude_pattern × normalization path (command_executed.py:142 any(exclude_re.search(h) for h in haystacks)) — the only direction that can LOWER a score; all eight pre-existing exclude_pattern tests (tests/test_command_executed.py:267-410) use bare, unwrapped commands, so the command_pattern + exclude_pattern + bash -lc "…'arg'…" combination that flips 1.0 → 0.0 below min_count ships untested. (trigger: tests/test_command_executed.py) (restates: Axis 7: Silent semantics change to the documented command_pattern/exclude_pattern task-author contract)
  • 🟠 The PR's stated design reason for putting normalization in the shared _matching_commands is that "the score and the live early-stop trigger stay consistent", yet no test exercises live_verdict with a wrapped/quoted command — the command_executed live-verdict block at tests/test_early_stop.py:438-490 uses only bare commands ("curl x", "ls", "rm -rf /"), so nothing pins score↔live parity under normalization or catches a future regression that normalizes in one path only. (trigger: tests/test_command_executed.py)
  • 🟡 Every degradation/edge branch of the new helper is unexercised — empty-token return, inner shlex.split ValueError, if inner: false, the non-wrapper break, and the _MAX_NORMALIZE_LEN skip are 100% of the module's uncovered lines (Missing: 54, 58->70, 63-64, 65->67, 68-69), and TestNormalizeShell tests only the outer parse failure. (trigger: tests/test_command_executed.py) (restates: Axis 3: Every new normalization degradation/edge branch is unexercised)
  • 🟡 TestNormalizeShell has no wrapper-shape matrix: no zsh -lc, sh -c, dash -c, or -ic case, and no argv-joined (unquoted) bash -lc uip is resources … case — the two shapes where the fix is silently inert or collapses the payload to its first word — so the wrapper allowlist and the unwrap loop can rot without any test failing. (trigger: tests/test_command_executed.py) (restates: Axis 8: bash -lc unwrap keeps only tokens[i+1], so an argv-joined payload collapses to its first word)
  • 🟡 The helper's own documented invariants have no tests: no case asserting a non-Bash record's JSON-serialized params are never normalized, no case for a Bash record whose command key is missing/empty (which currently DOES get its JSON blob shlex-tokenized), and no case for a non-str command value (Codex argv arrays), so the docstring contract at command_executed.py:80-81 is unguarded. (trigger: tests/test_command_executed.py) (restates: Axis 2: _match_haystacks re-derives shell-ness from tool_name == "Bash" alone, diverging from the caller's guard)
  • 🔵 No test pins either length window introduced/changed here — nothing asserts the raw haystack is still truncated at _MAX_PATTERN_SEARCH_LEN, that a command above _MAX_NORMALIZE_LEN skips normalization, or that the normalized haystack is capped — so the ReDoS bound and the (now asymmetric) match window are both free to drift. (trigger: tests/test_command_executed.py) (restates: Axis 7: Match window is asymmetric between the two haystacks)

Downstream consumers:

  • 🟠 The match count feeds the early-stop machinery added in #74 — the weighted ceiling/floor gate (armed_criteria_passed) and the max_steps_to_decide decision-step budget — and the in-tree regression tasks that exercise them (tasks/early_stop_weighted_high_weight_kills_run.yaml:41,48, tasks/early_stop_weighted_low_weight_absorbed.yaml:40,47, tasks/early_stop_decision_budget_exceeded.yaml:34) all use command_pattern with stop_when: auto, yet the PR neither re-ran them nor states whether a stop can now fire on a different step/verdict. (trigger: src/coder_eval/criteria/command_executed.py)
  • 🟠 Per-row command_executed scores roll up through BaseCriterion.aggregate() into suite means and suite_thresholds gates (which exit the CLI non-zero), so every dataset-backed suite that gates on this criterion can flip gate outcome in either direction from a pure-code change — no re-baseline step, threshold review, or migration note accompanies the change. (trigger: src/coder_eval/criteria/command_executed.py) (restates: Axis 7: Silent semantics change to the documented command_pattern/exclude_pattern task-author contract)

Display & mapping dicts:

  • 🟡 Nothing on the output surfaces records that a match (or a new exclusion) came from the normalized haystack: the display label at command_executed.py:146-149 and the Matched N/M details are built from raw text only, the filter summary at :283-288 echoes just the patterns, and _normalize_shell failures are never logged despite the module logger — so a task whose score moved because of this PR is undiagnosable from task.json/reports or coder-eval review. (trigger: src/coder_eval/criteria/command_executed.py)

Daily/nightly:

  • 🟠 The commit message's guarantee — "This is additive — nothing that matched before can stop matching" — is empirically false (exclude_pattern widening can drop a command below min_count, and the widened any() can trip max_count: 0), and since CHANGELOG.md is auto-generated from conventional commits, that false guarantee is what the out-of-repo coder-eval-uipath suite owners (whose ~173/1557 quote-tolerant criteria are the actual blast radius) will read; the PR states no nightly impact, no re-baseline plan, and offers no opt-out flag. (trigger: src/coder_eval/criteria/command_executed.py) (restates: Axis 7: Silent semantics change to the documented command_pattern/exclude_pattern task-author contract)
  • 🟡 Nightly stop_early runs now pay a pure-Python shlex lex of every accumulated Bash command on every ToolStartEvent and resolved ToolEndEvent inside EarlyStopWatcher._evaluate (~2 full re-scans per tool call, ~3.1 ms/command at the 20 000-char cap vs ~0.0007 ms before) — a production-path cost increase the PR does not mention or bound. (trigger: src/coder_eval/criteria/command_executed.py) (restates: Axis 4: shlex.split on up to 20 000 chars inside the per-tool-call early-stop callback)
  • 🟡 Because the wrapper allowlist omits zsh, identical agent behavior now scores differently depending on the host shell of the machine running the suite (Codex uses zsh -lc on macOS, bash -lc on Linux CI/Docker) — the PR does not state that nightly scores become host-dependent, which breaks local-vs-nightly comparability. (trigger: src/coder_eval/criteria/command_executed.py) (restates: Axis 1: Shell-unwrap allowlists _SHELL_WRAPPERS/_SHELL_CMD_FLAGS are incomplete/inconsistent (zsh, -ic missing))

Harness & Lint Improvements

Static checks (lint / type):

  • [pyright] Change CommandTelemetry.parameters (and its sibling model) from dict[str, Any] to dict[str, object] at /Users/religa/src/coder_eval/src/coder_eval/models/telemetry.py:367 and :435. This is the option-(2) pyright tightening, preferred over a custom rule because it makes the defect a hard type error at every read site instead of relying on a pattern match. VERIFIED with a throwaway probe (own pyrightconfig, standard mode, py3.13): with dict[str, object], txt = t.parameters["command"]; shlex.split(txt, posix=True) yields error: Argument of type "object" cannot be assigned to parameter "s" of type "str | _ShlexInstream" plus error: "__getitem__" method not defined on type "object" for the slice — exactly the two operations the finding flags — while pydantic round-trips the value untouched (T(parameters={"command": ["bash","-lc","ls"], "n": 3}).model_dump_json() -> {"parameters":{"command":["bash","-lc","ls"],"n":3}}; no validation or wire-format change). Blast radius is small and enumerable: 17 read sites in 8 files (command_executed.py 6, skill_triggered.py 2, reports.py 2, reports_html.py 2, streaming/renderers.py 2, judge_context.py 1, summaries.py 1, antigravity_agent.py 1), each needing the isinstance(..., str) narrow that criteria/skill_triggered.py:67-71 already demonstrates. Land this instead of a CEnnn rule for the Any-leak class: pyright cannot see the bug today precisely because Any is assignable to str, and no existing config flip reaches it (no ruff rule; strict/reportUnknown* does not fire because Any is known, not unknown). Prevents: A2/high — cmd_text = cmd.parameters["command"] (command_executed.py:127) passing a list into shlex.split (line 50) and into the str-annotated params at lines 32/73, flipping a pattern-less command_executed criterion from 1.0 to 0.0 with AttributeError: 'list' object has no attribute 'read' and poisoning every other command in the trajectory. Also pre-empts the recurrence class: any future untyped SDK-parameter read reaching a str-only stdlib call.
  • [ce-lint] New CEnnn rule (claim the next free id — implemented rules stop at CE031 and CE026/CE032/CE033 are already reserved in .claude/harness-candidates.md, so CE034 is the first safe number): forbid literal tool_name == "Bash" / != "Bash" comparisons anywhere under src/coder_eval/ outside one named predicate (add CommandTelemetry.is_shell on the model and require its use). File as tests/lint/rules/ce034_no_tool_name_magic_string.py as a BaseRule subclass, wired into the import block + ALL_RULES list in tests/lint/runner.py:12-41, following ce018_no_final_status_name_denylist.py (which forbids the structurally identical "membership test against a magic status string" pattern). Grep-verified violations today, each pairing the same == "Bash" test with a DIFFERENT companion guard — precisely the drift the finding is about: criteria/command_executed.py:84 (and 0 < len(cmd_text) <= _MAX_NORMALIZE_LEN), :126 and :146 (and cmd.parameters.get("command")), evaluation/summaries.py:33 and evaluation/judge_context.py:385 (both and "command" in params). Add a second violation class in the same rule file: direct .parameters[...] / .parameters.get(...) reads outside models/telemetry.py, forcing extraction through one narrowing accessor (CommandTelemetry.shell_command() -> str | None). That class is the cheap, sound stand-in for the much harder general rule ("an Any-typed dict value reaching a str parameter without an isinstance narrow"), and it complements the pyright change: the type change fixes the Any leak but cannot see guard divergence. Prevents: A2/medium — _match_haystacks (command_executed.py:84) re-deriving shell-ness from tool_name alone while its caller (line 126) also requires a non-empty command, so a Bash record with a missing/empty command gets its JSON-serialized params shlex-normalized ({"description": "run the pytest suite"} -> extra haystack {description: run the pytest suite}), contradicting the helper's own docstring and flipping an exclude_pattern criterion from 1.0 to 0.0. Also the four other divergent copies of the guard listed above.
  • [ce-lint] Extend the existing CE030 doc/schema-parity check (tests/lint/doc_schema_parity.py, wired as tests/test_custom_lint.py::TestCE030DocSchemaParity) to the members of the SuccessCriterion union, paired with docs/TASK_DEFINITION_GUIDE.md. Verified gap: grep -c 'exclude_pattern\|command_pattern' docs/TASK_DEFINITION_GUIDE.md returns 2, both of which are command_patternexclude_pattern is documented NOWHERE in the guide, yet this PR changes what it excludes. DOCUMENTED_MODELS (doc_schema_parity.py:45-52) registers only TaskDefinition/RunLimits/Dataset/SimulationConfig, and the module docstring makes excluding criteria a deliberate choice ("Explicit registry, no recursion... Walking them would silently expand the documentation commitment to dozens of models nobody signed up for"). So this is a conscious, argued expansion, not an oversight fix: the guide already claims to be the criterion reference (all 14 types have sections), the union is enumerable rather than open-ended, and framework-set fields go in EXEMPT with a stated reason. Scope to union members only (no recursion into nested models) so the promise stays bounded. Limitation to record alongside it: CE030's inline-code match catches an ENTIRELY undocumented field (exclude_pattern), not a semantics change to an already-documented one — for that, see the golden-corpus harness item. Prevents: A7/high — the silent bidirectional re-scoring of unedited task YAML (command_pattern/exclude_pattern now match a second, shlex-normalized haystack) shipping with the author-facing contract untouched: the Field descriptions at models/criteria.py:595-596 and :617-620 and the guide at docs/TASK_DEFINITION_GUIDE.md:810 never mention normalization, and nothing mechanically gated the drift. Also A6/low (a normalization rationale, with now-false invariants, restated four times in private docstrings instead of on the documented surface).
  • [ce-lint] New CEnnn rule (next free id after the above, e.g. CE035) as tests/lint/rules/ce035_frozen_membership_constants.py, wired into tests/lint/runner.py: a module-level private constant assigned a set literal must be an annotated frozenset — flag _NAME = {"a", "b"} at module scope under src/coder_eval/, require _NAME: frozenset[str] = frozenset({"a", "b"}). Cheap to land: the whole tree has exactly 3 violations today (criteria/command_executed.py:28 and :29, both new in this PR, plus the pre-existing agents/_logging.py:22 _TRUTHY), against a convention the rest of the codebase already follows uniformly (agents/codex_agent.py:163, models/results.py:240, isolation/docker_runner.py:66, agents/antigravity_agent.py:127). No existing ruff rule reaches this: RUF012 covers only mutable class attribute defaults, and the already-enabled RUF/B/SIM sets produced no diagnostic here. Prevents: A2/low — _SHELL_WRAPPERS = {"bash", "sh"} and _SHELL_CMD_FLAGS = {"-c", "-lc", "-lic"} shipping as mutable, unannotated set[str] module constants that an importer can mutate, against the repo's uniform frozenset convention.
  • [ce-lint] Weakest of the five — include only if the shell-unwrap keeps an enumerated constant at all: a CEnnn SSOT rule forbidding shell PROGRAM-NAME string collections outside one canonical module (e.g. coder_eval/shell.py) — flag any set/frozenset/tuple/list literal containing "bash" or "sh" elsewhere under src/coder_eval/. The root cause of the zsh omission is duplicated knowledge: the criterion hardcodes {"bash", "sh"} while the Codex agent already documents the true domain in prose (agents/codex_agent.py:1130: "bash -lc on Linux, zsh -lc on macOS") and the sibling test suite already models real /bin/zsh -lc telemetry (tests/test_skill_triggered.py:319). A shared constant makes the criterion consult that fact instead of re-guessing. Caveat stated deliberately: a linter cannot know that zsh or -ic is MISSING from an allowlist — that is semantic. If the finding's preferred fix is taken (replace both enumerated allowlists with a predicate: basename endswith("sh") plus "first --prefixed token whose option letters include c"), the constants disappear and this rule is moot; then skip it and rely on the wrapper-shape fixture matrix in the harness bucket, which is the guard that actually reaches this class. Prevents: A1/medium (cross-axis 1/2/5/7/8) — _SHELL_WRAPPERS/_SHELL_CMD_FLAGS omitting zsh and -ic, so zsh -lc "..." (Codex's shape on macOS) and bash -ic "..." are never unwrapped and the PR's fix is silently inert on those hosts: identical agent behavior scores differently by host shell, with no diagnostic (the criterion just returns 0.0 again).

Harness improvements (not statically reachable):

  • Criterion-scoring golden/characterization corpus. Add tests/data/criterion_scoring_golden.json (or a parametrized fixture table) pinning (CommandTelemetry fixture, criterion config) -> exact score for the command-matching criteria, asserted verbatim. Any change to matching semantics then fails loudly on unchanged inputs and must be re-baselined in the same commit, making a score-direction change an explicit reviewable diff instead of an invisible one. Seed it with the two verified flip cases from this review: /bin/bash -lc "'uip' or users list" + command_pattern: uip\s+or\s+users\s+list, min_count: 0, max_count: 0 (main 1.0 -> PR 0.0), and command_pattern: uip\s+run + exclude_pattern: foo\s+--dry-run, min_count: 1 on /bin/bash -lc "uip run 'foo' --dry-run" (main 1.0 -> PR 0.0 — the case that disproves the PR's own "nothing that matched before can stop matching" claim). Why not static: A verdict change is a property of executing the checker against telemetry; no AST or type analysis can tell that a regex now matches a second haystack and therefore re-scores YAML nobody edited. CE030 (static bucket) catches an undocumented field, never a semantics change to a documented one. Prevents: A7/high (silent bidirectional re-scoring of unedited task YAML, blast radius in the out-of-repo coder-eval-uipath suites), A7/low (haystack window asymmetry: a 2426-char command with its target at offset 2415 goes 0.0 -> 1.0 because normalization compresses it inside the 2000-char cap), A2/medium (the Bash-without-command exclude flip).
  • Real-telemetry wrapper-shape fixture matrix. One shared parametrized corpus of CAPTURED command strings covering every shape the agents actually emit, run against every command-matching criterion (command_executed, skill_triggered) and against _normalize_shell directly: Claude Bash{command: "..."}; Codex primary commandExecution quoted display strings (/bin/bash -lc "... 'arg' ..." AND /bin/zsh -lc "..." — the macOS default, already present at tests/test_skill_triggered.py:319); Codex sub-agent rollout recovery, which joins argv WITHOUT re-quoting (agents/codex_agent.py:1928-1929), i.e. bash -lc uip is resources run list ... --output json; bash -l -c, bash -ic, dash -c; empty payload (bash -lc ""); a Bash record with the command key absent (agents/codex_agent.py:1932 dict(action)) and with command == "" (:2051). Assert the resulting haystack LIST, not just the score, so a degenerate result is visible. Why not static: The set of wrapper forms is a property of the host shell plus each agent runtime's telemetry encoding, not of this repo's source — enumerating it requires recorded agent output. A linter can flag a hardcoded allowlist (static bucket) but cannot know which members are missing. Prevents: A8/high (argv-joined payload collapsing to its first word — _normalize_shell("bash -lc uip is resources run list ...") returns just 'uip', so the fix is a silent no-op on the sub-agent path and the degenerate one-word haystack can newly match an ^-anchored pattern), A1/medium (zsh / -ic never unwrapped), A2/medium (Bash record with no command), A3/medium (the five uncovered branches), A6/low (_normalize_shell returning None for benign-empty vs parse-error indistinguishably, and 'bash -lc ' for an empty payload).
  • Per-changed-file branch-coverage gate. make verify runs a single global --cov-fail-under=80 (Makefile:57), which passed comfortably while 100% of this module's misses were the new code's own error paths: command_executed.py measured 93.57% with Missing: 54, 58->70, 63-64, 65->67, 68-69 — every one inside _normalize_shell. Add a diff-scoped gate (diff-cover over the PR diff, or --cov-fail-under=100 applied to modules touched by the diff) as a new make target wired into CI, so newly added branches must be exercised regardless of how healthy the global number looks. Why not static: Needs test execution plus coverage data plus the PR diff; coverage is a runtime measurement, not a source property. Prevents: A3/medium (every new degradation/edge branch unexercised: inner ValueError, empty-token, non-wrapper break, _MAX_NORMALIZE_LEN skip). Would also have surfaced the A8/high argv-joined gap and the A1/medium zsh gap as concrete coverage holes at review time.
  • Hot-path re-computation guard for live_verdict. Assert — via a counting monkeypatch, not wall-clock timing, so it cannot flake — that _normalize_shell runs at most once per CommandTelemetry per run: memoize the normalized form on the telemetry record (or compute it at capture time) and pin that with a test. Today EarlyStopWatcher._evaluate (orchestration/early_stop.py:373) calls live_verdict -> _matching_commands over the WHOLE accumulated trajectory on every ToolStartEvent and every resolved ToolEndEvent (on_event, lines 291-341), so cost is quadratic in tool calls; measured through the real path, a 200-command scan of 20000-char commands went from 0.14 ms (main) to 620.60 ms (PR HEAD) — ~3.10 ms/command, ~4400x — accumulating to ~125 s of synchronous CPU inside a StreamCallback on the agent's asyncio loop. Why not static: An algorithmic-complexity property spanning two modules and depending on runtime state (accumulated trajectory length x event count); no AST rule can see that a linear helper is called O(n^2) times from a callback in another package. Prevents: A4/low (CVSS:3.1/AV:L/AC:L/PR:L/UI:N/S:U/C:N/I:N/A:L) — an evaluated agent emitting large heredoc/script commands stalling a stop_early run. Also removes the incentive for the _MAX_NORMALIZE_LEN cap whose off-by-one silently changes pattern semantics at 20001 chars (A1/low).
  • Make silent criterion degradation observable in run artifacts. When a criterion's matching path degrades (normalization returned None, a wrapper was recognized but not unwrapped, a command parameter was non-str), emit a logger.debug/warning at the degrade site and surface a per-run counter in the report, so an inert fix or a poisoned trajectory is distinguishable from a legitimate 0.0. Every failure mode in this PR presents identically to a genuine miss: zsh -lc unstripped -> 0.0; argv-joined payload -> useless one-word haystack; list command -> caught AttributeError that zeroes the whole criterion. Pairs with splitting _normalize_shell's overloaded None (parse error vs benign empty vs inner parse error) so the log can say which happened. Why not static: A linter cannot distinguish a legitimate 0.0 from a degraded one — that needs runtime observability of the degrade path and somewhere in the run record to report it. Prevents: A1/medium (zsh/-ic fix silently inert), A6/low (overloaded None sentinel; the inner except ValueError discarding an already-successful outer normalization), A2/high (one non-str command aborting _matching_commands for the entire trajectory with no operator-visible signal), A8/high (argv-joined no-op).
  • Criterion-semantics change protocol (process; one line in CONTRIBUTING plus the PR template). A change to how any criterion scores must state, in the commit body, the direction(s) in which unchanged task YAML can re-score, and flag cross-repo re-baselining for the external coder-eval-uipath / eval-runner suites. Motivation is concrete: commit b2f5401's body asserts "This is additive - nothing that matched before can stop matching", which is empirically false (a newly-matching exclude_pattern drops a command below min_count: main 1.0 -> PR 0.0), and because CHANGELOG.md is auto-generated from conventional commits, that false guarantee ships verbatim to exactly the downstream suite owners who must re-baseline. tasks/ contains only one command_pattern (tasks/agents/subagent_bash_long_input.yaml:74), so in-repo tests structurally cannot represent the real blast radius. Why not static: Requires semantic comparison of a prose claim against runtime behavior, plus knowledge of consumers that live in a different repository. The golden corpus above is the mechanical half of this pair; this is the irreducibly human half. Prevents: A7/high (undocumented, un-announced contract change with no opt-out) and the false CHANGELOG guarantee derived from it.
  • Record the deliberately-unmechanizable residue in .claude/harness-candidates.md under an explicit "reviewer-only (no gate proposed)" heading, so a future review does not re-litigate: (a) _MAX_NORMALIZE_LEN = 10 * _MAX_PATTERN_SEARCH_LEN — a self-admittedly unnecessary guard whose value is coupled to an unrelated ReDoS constant — plus the dead 0 < len(cmd_text) companion (verified: _normalize_shell("") and _normalize_shell(" ") both return None, which the next line already handles); (b) the four docstrings/comments restating a normalization rationale whose stated invariants are provably false (heredocs parse fine — shlex.split('bash -lc "cat <<EOF\\nhello\\nEOF"') succeeds — and normalization CAN remove a match via exclude_pattern). This matches the repo's own precedent: .claude/harness-candidates.md already parks deferred guardrails with a reason and an id reservation. Why not static: Both are semantic-judgment classes. "This constant is derived from an unrelated one" and "this docstring's invariant is false" require understanding intent; a rule broad enough to flag them (banning derived numeric constants, or diffing prose against behavior) would be pure noise. Recording the boundary makes it a decision rather than an omission. Prevents: A1/low (_MAX_NORMALIZE_LEN over-engineering plus the dead length check), A6/low (false docstring invariants and their fourfold duplication).

Top 5 Priority Actions

  1. Narrow the command extraction at src/coder_eval/criteria/command_executed.py:127 with isinstance(raw, str) (the pattern already used at criteria/skill_triggered.py:67-71), because Codex sub-agent telemetry can carry command as an argv list and shlex.split then raises AttributeError: 'list' object has no attribute 'read', zeroing an entire pattern-less command_executed criterion that scored 1.0 on main.
  2. Fix the wrapper unwrap at src/coder_eval/criteria/command_executed.py:61-67 to rejoin the remaining tokens (tokens[i+1:]) instead of keeping only tokens[i+1], since Codex's rollout-recovery path joins argv without re-quoting (agents/codex_agent.py:1928-1929) so bash -lc uip is resources ... normalizes to just 'uip' — the fix is a silent no-op there and the degenerate one-word haystack can newly match an anchored pattern, flipping a min_count: 0/max_count: 0 negative assertion to 0.0.
  3. Replace the enumerated allowlists at src/coder_eval/criteria/command_executed.py:28-29 ({"bash","sh"} / {"-c","-lc","-lic"}, which omit zsh and -ic while listing the exotic -lic) with a generic predicate — shell-like basename plus the first --flag containing c — declared as annotated frozensets per repo convention, so the fix is not silently inert on macOS Codex hosts that shell through zsh -lc (agents/codex_agent.py:1130), and add one test per wrapper form so the set cannot rot.
  4. Make the helper total rather than re-deriving caller state: change the signature at src/coder_eval/criteria/command_executed.py:73 to _match_haystacks(cmd_text: str, *, is_shell: bool) with is_shell computed once at line 126 (line 84's tool_name == "Bash"-only gate today shlex-tokenizes a JSON params blob when command is missing/empty, flipping an exclude_pattern case from 1.0 to 0.0 against the helper's own docstring), normalize the already-truncated cmd_text[:_MAX_PATTERN_SEARCH_LEN] so both haystacks share one match window (line 83 vs 84-87 currently lets patterns see text past the documented 2000-char cap), memoize the normalized form per telemetry record since EarlyStopWatcher._evaluate (orchestration/early_stop.py:373) re-lexes the whole trajectory ~2x per tool call (~3.1 ms/command vs 0.0007 ms on main), and add the ~5 one-line tests covering the currently-uncovered branches (lines 54, 58->70, 63-64, 65->67, 68-69).
  5. Close the contract gap: document raw-OR-normalized matching (and that exclude_pattern now also excludes quote-obfuscated forms) in both Field descriptions at src/coder_eval/models/criteria.py:595-596 and 617-620 plus the command_executed section of docs/TASK_DEFINITION_GUIDE.md:810, correct the commit message so the auto-generated CHANGELOG does not ship the disproven "This is additive — nothing that matched before can stop matching" guarantee (a newly-matching exclude_pattern drops a command below min_count: main 1.0 → PR 0.0), and extend CE030-style doc/schema parity (tests/lint/doc_schema_parity.py:45-46) to SuccessCriterion models so the next criterion-semantics change cannot ship undocumented.

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

@uipreliga
uipreliga self-requested a review August 4, 2026 15:02

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Fix what you agree with and 🚢

rockymadden and others added 10 commits August 5, 2026 08:14
…lizing

`CommandTelemetry.parameters` is `dict[str, Any]`, and a `command` value is
not guaranteed to be a `str`: Codex sub-agent rollout recovery can carry it as
an argv *list*. That list reached `shlex.split`/slicing and raised
`AttributeError: 'list' object has no attribute 'read'`, which aborted
`_matching_commands` for the entire trajectory and zeroed an otherwise-passing
`command_executed` criterion.

Narrow the extraction (and the display-label) site with `isinstance(raw, str)`
— mirroring criteria/skill_triggered.py — so a non-str `command` falls back to
the JSON-serialized params blob instead of crashing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `bash -lc` unwrap kept only `tokens[i+1]`, so an argv-joined (unquoted)
payload collapsed to its first word: Codex sub-agent rollout recovery joins
argv WITHOUT re-quoting (codex_agent.py), so `bash -lc uip is resources ...`
normalized to just `uip` — making the shell-normalization fix a silent no-op
on that path, and letting the degenerate one-word haystack newly (and falsely)
satisfy an anchored pattern like `^uip$`.

Rejoin everything after the -c/-lc flag: re-split the single quoted-script
token to resolve inner quotes, but keep every token of the already-split
argv-joined form. Also strips the wrapper cleanly for an empty script
(`bash -lc ""` -> `''` instead of the odd `bash -lc `).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…owlist

The enumerated `_SHELL_WRAPPERS = {"bash","sh"}` / `_SHELL_CMD_FLAGS =
{"-c","-lc","-lic"}` sets omitted `zsh` (Codex's login shell on macOS, see
codex_agent.py) and the common `-ic` while listing the exotic `-lic`. On those
hosts the shell-normalization silently reverted to the pre-fix false-negative
behaviour, so identical agent behaviour scored differently by host shell.

Replace both enumerated sets with predicates over the open shell/flag domain:
`_is_shell_program` matches any basename ending in `sh` (bash/sh/zsh/dash/ksh);
`_is_command_flag` matches a short-option cluster whose letters are alphabetic
and include `c` (`-c`/`-lc`/`-ic`/`-lic`, and the split `bash -l -c` form),
while rejecting `--long` options and `git -c <config>`. Matching stays additive
(raw text remains a haystack), so favouring recall is safe. Adds one test per
wrapper form so the domain cannot silently rot.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…moized

Make _match_haystacks total and stop it re-deriving caller state:

- Signature is now `(cmd_text, *, is_shell)`; `is_shell` is decided once at the
  single extraction site (a Bash tool whose `command` is a non-empty `str`).
  Previously the helper re-derived shell-ness from `tool_name == "Bash"` alone,
  so a Bash record with a missing/empty `command` had its JSON params blob
  shlex-tokenized — the stripped JSON quotes could newly satisfy an
  `exclude_pattern` and drop the command below `min_count` (flip 1.0 -> 0.0).
- Normalize the already-truncated `cmd_text[:_MAX_PATTERN_SEARCH_LEN]` so both
  haystacks describe the same 2000-char window; quote-stripping can no longer
  slide content from past the cap into the match. This also bounds `shlex` input
  for free, retiring the `_MAX_NORMALIZE_LEN` guard (whose value was coupled to
  an unrelated ReDoS constant and whose off-by-one silently changed semantics at
  the cap).
- Memoize `_normalize_shell` (pure function): `EarlyStopWatcher._evaluate`
  re-scans the whole accumulated trajectory on every tool-call event, so the
  same command was lexed O(n^2) times per run — the cache collapses that to
  once per distinct command string.

Tests: cover every previously-uncovered `_normalize_shell` branch (empty/
whitespace input, inner unbalanced-quote fallback, non-wrapper positional,
flags-without-command), the is_shell divergence (Bash-without-command not
excluded), the shared-window guard, and memoization (via cache_info, not
timing). Module is now at 100% statement + branch coverage.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…it (CE030)

The shell-normalization change altered what `command_pattern`/`exclude_pattern`
match on unedited task YAML, but the author-facing contract was untouched.

- `command_pattern` / `exclude_pattern` Field descriptions now state that a Bash
  command is matched against the raw text OR its shell-normalized (shlex-resolved,
  wrapper-stripped) form, and that `exclude_pattern` therefore also excludes
  quote-obfuscated variants.
- TASK_DEFINITION_GUIDE gains a "Shell normalization" paragraph under
  `command_executed` spelling out the raw-OR-normalized behavior and, crucially,
  that it is NOT purely additive: a normalized haystack can newly satisfy an
  exclusion or a `max_count: 0` gate, so cross-repo suites that hand-encoded
  quote tolerance should re-baseline. Also fills in the previously YAML-only
  field references (command_executed / reference_comparison field tables,
  run_command `score_from_stdout`, the judges' `capture_transcript` /
  `max_transcript_chars`).
- Corrects the `_normalize_shell` docstring's false invariants (heredocs parse
  fine; matching is not purely additive).
- Extends CE030 doc/schema-parity to the `SuccessCriterion` union members
  (enumerated, no recursion), so the next criterion-semantics change cannot ship
  with an undocumented field. Registry now covers 18 models, 0 undocumented.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The CE030 extension flattened the whole `SuccessCriterion` union, but a plugin
can inject its own criterion into that union at load time via the
`coder_eval.plugins` hook — the `uipath` SDK (installed by CI's `--extra uipath`)
adds a `CliCalledCriterion` (fields `log`/`positional`). CE030 then demanded the
in-repo guide document a criterion this repo doesn't own, failing CI's Quality
Gate (the failure did not reproduce on macOS, whose lockfile resolution omits
the injecting package).

Scope `_criterion_models()` to criteria defined in `coder_eval.models.criteria`
(filter on `__module__`), and read the union live from the module so plugin
reassignment before import is still handled. Plugin-contributed criteria are
documented in their own repos, not this guide.

Adds two regression tests: the enumerated members are all in-tree, and a
plugin-injected (foreign-`__module__`) criterion is excluded from the parity
check.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous CE030 scope fix filtered union members by ``__module__``, but the
injected ``CliCalledCriterion`` is built with ``pydantic.create_model`` and a
spoofed ``__module__="coder_eval.models.criteria"``, so the string filter did
not exclude it and CI still failed.

Enumerate the concrete ``BaseSuccessCriterion`` subclasses that are genuine
module-level attributes of ``coder_eval.models.criteria`` instead of flattening
the ``SuccessCriterion`` union at all. A ``create_model`` class is not inserted
into the module namespace, so union injection can't contaminate the set, while
every criterion this repo actually defines is still covered automatically.

The regression test now mirrors the real shape (create_model + spoofed
``__module__`` + union splice, never a module attribute) and asserts exclusion.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…untime

The `uipath` SDK's `coder_eval.plugins` hook integrates its `CliCalledCriterion`
so thoroughly at load time — real module attribute, `__module__` spoofed to
`coder_eval.models.criteria`, and spliced into the runtime `SuccessCriterion`
union — that neither a `__module__` filter nor a module-attribute scan could
tell it apart from an in-tree criterion. Every runtime signal is contaminated.

Read the union members from the `SuccessCriterion = Annotated[...]` literal in
the criteria.py SOURCE (AST parse), then resolve each name via getattr. A plugin
cannot edit this repo's source, so the list is exactly the criteria this repo
ships — and a new in-tree criterion is still covered automatically (it's added
to the source literal). The regression test now reproduces the full injection
shape (module attr + union splice + spoofed __module__) and asserts exclusion.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e in CI)

CI installs `--extra uipath`, and in that environment `coder_eval.models.criteria`
gains a `CliCalledCriterion` (fields `log`/`positional`) absent from a plain
checkout — it did not reproduce on macOS. It defeated every way tried to tell it
apart from an in-tree criterion: union membership, a `__module__` string filter
(spoofed to the in-tree module), a genuine-module-attribute scan (it is setattr'd
onto the module), and an AST parse of the union literal in criteria.py source (CI's
criteria module resolves to a file whose literal already contains it).

Restore CE030 to the original four top-level models. The `command_pattern` /
`exclude_pattern` contract this PR changed remains documented in the Field
descriptions and TASK_DEFINITION_GUIDE; the union extension is parked in
.claude/harness-candidates.md with the full diagnosis for a future revisit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rockymadden
rockymadden merged commit 7abd080 into main Aug 5, 2026
12 checks passed
@rockymadden
rockymadden deleted the fix/command-executed-shell-normalize branch August 5, 2026 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants