feat(protected-mock): add subset fixture matching and harden mockd startup - #93
feat(protected-mock): add subset fixture matching and harden mockd startup#93dmorosanu wants to merge 1 commit into
Conversation
|
Claude finished @dmorosanu's task in 1m 36s —— View job Code Review in Progress
|
|
Live validation against the real skills #2503 corpus (codex/remove-runtime-seal @ efd1fe6e2) on the Linux VM, image rebuilt from this branch, docker driver with agent isolation, Bedrock claude-sonnet-4-6:
Two rollout blockers found in the corpus x harness combination (not defects of this diff, filed for the #2503 migration):
|
uipreliga
left a comment
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:93 (4 files) axis:1,2,3,4,5,6,7,8
Scope: pr:93 (4 files) axis:1,2,3,4,5,6,7,8 · branch feat/protected-mock-subset-matching · 7413f3f · 2026-08-08T05:48Z · workflow variant
Change class: complex — changes argv-normalization control flow and adds a new subset match mode to the protected-mock isolation server, plus subprocess stderr capture/lifecycle changes; correctness requires reasoning about matching precedence and security boundaries
Security is clean (10/10) and the core architecture, typing, and API surface stay strong (9.1/10 overall), but the new protected_mock subset match mode ships with three grading-relevant hazards — rule argv silently re-normalized, subset silently outranking passthrough, and mid-run mockd death mis-attributed to the agent — none of which are pinned by tests (Test Health 8.3, Error Handling 8.3); bottom line: land the subset-matching and mockd-lifecycle fixes with regression tests before any task fixture adopts match_mode: subset, and the rest is polish.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 9.2 / 10 | 0 | 0 | 1 | 3 | Diff pushes _load_tool to CC 20 and dispatch to CC 12 by inlining the subset branch and scan |
| 2. Type Safety | 9.3 / 10 | 0 | 0 | 1 | 2 | Fixture entries are hand-parsed Any dicts with no extra="forbid": a typo'd match_mode key silently degrades a subset rule to exact matching (so every argv variant falls through to default), and a non-hashable match_mode raises TypeError at the set-membership test instead of the descriptive ValueError one line below |
| 3. Test Health | 8.3 / 10 | 0 | 1 | 1 | 2 | New subset match mode has no test for a rule containing a flag+value, so the position-free flag/value decoupling (--job-id 42 matching --job-id 99 --tag 42) is unpinned |
| 4. Security | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 5. Architecture & Design | 9.5 / 10 | 0 | 0 | 1 | 0 | Stale "exact-command fixture service" claim in server.py docstring and sibling doc/model sites after subset mode |
| 6. Error Handling & Resilience | 8.3 / 10 | 0 | 1 | 1 | 2 | Post-startup mockd exit is never checked and the captured stderr is unlinked unread (runtime.py:84-96) |
| 7. API Surface & Maintainability | 9.4 / 10 | 0 | 0 | 1 | 1 | Subset rules silently shadow passthrough_argv_prefixes: precedence is neither documented nor tested |
| 8. Evaluation Harness Quality | 9 / 10 | 0 | 1 | 0 | 0 | Subset rule argv is run through the invocation-side noise-flag normalizer, silently widening/eating rule tokens (and the docs describe normalization as invocation-only) |
Overall Score: 9.1 / 10 · Weakest Axis: Test Health at 8.3 / 10
Totals: 🔴 0 · 🟠 3 · 🟡 6 · 🔵 10 across 8 axes.
Blockers
- [Axis 3] New
subsetmatch mode has no test for a rule containing a flag+value, so the position-free flag/value decoupling (--job-id 42matching--job-id 99 --tag 42) is unpinned (tests/test_protected_mock.py:213) — All four new subset tests use rules made only of bare positional tokens —{"argv": ["rpa", "get-errors"], "match_mode": "subset"}(line 216),["rpa"](233),["rpa", "get-errors"](234),["rpa", "list-jobs"](256). None covers a rule with a flag and its value, which is the shape the docs advertise (docs/TASK_DEFINITION_GUIDE.md: "matches when every rule token appears in the invocation's normalized token set") and the shape the siblingnormalizedmode is authored with (["rpa", "get-errors", "--job-id", "42"], line 151). Because server.py:218-221 matches against an unorderedset(_expand_argv_tokens(argv))withall(token in invocation_tokens for token in rule_tokens), the flag and its value are decoupled. Verified at PR head: a subset rule["rpa","get-errors","--job-id","42"]returns its canned stdout for the invocation["rpa","get-errors","--job-id","99","--tag","42"](job 99's errors answered with job 42's fixture) and for["rpa","get-errors","42","--job-id","7"]. Add tests asserting the intended contract for flag-bearing subset rules — at minimum a rule with--job-id 42that must NOT match an invocation carrying--job-id 99, plus the--job-id=42inline form and a non-noise empty-value inline flag (--job-id=, the still-uncovered58->50branch). If the permissive semantics are intentional, a test must pin them explicitly and the guide must say the value is matched position-free, because this decides which fixture answer an agent receives and therefore the task's score. - [Axis 6] Post-startup mockd exit is never checked and the captured stderr is unlinked unread (runtime.py:84-96) (
src/coder_eval/protected_mock/runtime.py:85) —process.returncodeis consulted at exactly one place — line 72, inside the startup poll (grep -n "returncode" src/coder_eval/protected_mock/runtime.py→ only line 72). Onceyield(line 84) returns, thefinallyruns:
finally:
if process.poll() is None:
process.terminate()
...
with contextlib.suppress(OSError):
os.unlink(stderr_path)If mockd died at minute 2 of a 10-minute run, process.poll() is None is False, so the branch is skipped, and line 96 unlinks the stderr file without ever reading it — _server_stderr_suffix is only called at lines 73 and 82 (startup). The agent side degrades silently: client.py::invoke catches the connect OSError and returns exit 125 per call, so the task simply scores as an agent failure with no harness-level signal and the traceback this PR just started capturing is destroyed. Fix: in the finally, when the body completed but process.poll() is not None, log at ERROR with _server_stderr_suffix(stderr_path) (and ideally set a non-success run status) before unlinking, so a mid-run mockd crash is distinguishable from an agent failure. n/a
3. [Axis 8] Subset rule argv is run through the invocation-side noise-flag normalizer, silently widening/eating rule tokens (and the docs describe normalization as invocation-only) (src/coder_eval/protected_mock/server.py:128) — _load_tool builds a subset rule's token set with the same order-sensitive noise-flag scanner used for invocations:
128: rule_tokens = tuple(_expand_argv_tokens(argv))
129: if not argv or not rule_tokens:
130: raise ValueError(f"fixture {fixture_path} response {index}.argv must be non-empty for subset matching")and _expand_argv_tokens swallows the token after --output whenever it is not flag-shaped:
68: if token in _NOISE_VALUE_FLAGS:
71: if index < len(expanded) and not expanded[index].startswith("-"):
72: index += 1
73: continueI executed the shipped logic against several rule shapes; --output eats the following rule token, not just a format value:
['--output', 'rpa', 'get-errors'] -> ['get-errors']
['rpa', '--output', 'get-errors'] -> ['rpa']
So {"argv": ["--output", "rpa", "get-errors"], "match_mode": "subset"} is loaded as the one-token rule ('get-errors',), which then matches ANY invocation containing get-errors anywhere. The not rule_tokens guard on line 129 only fires when every token is consumed (tests/test_protected_mock.py:280-284 covers exactly that all-or-nothing case); partial consumption is silent — no error, no warning. This directly contradicts docs/TASK_DEFINITION_GUIDE.md:584, which tells the author subset matching works "regardless of order", so re-ordering a rule's tokens is presented as safe when it changes which invocations resolve to that canned response.
Fix: do not run the value-swallowing pass over subset rule argv — for a subset rule, order is meaningless by definition, so normalize it as a pure set: split --flag=value, drop bare _NOISE_VALUE_FLAGS tokens and their inline values, and drop nothing else. Alternatively, reject a subset rule at load if len(_expand_argv_tokens(argv)) < len([t for t in argv if t not in _NOISE_VALUE_FLAGS and not t.split('=',1)[0] in _NOISE_VALUE_FLAGS]) so a silently-narrowed rule fails loudly instead of quietly widening. Add a test asserting ["--output", "rpa", "get-errors"] and ["rpa", "get-errors", "--output", "json"] load to the same two-token rule.
Non-blocking, but please consider before merge
- [Axis 1] Diff pushes
_load_toolto CC 20 anddispatchto CC 12 by inlining the subset branch and scan (src/coder_eval/protected_mock/server.py:100) — Measured withradon cc -son the base and head blobs of this file:_load_toolC(17) -> C(20) andProtectedMockServer.dispatchB(7) -> C(12) (_normalized_argvA(1) with the new_expand_argv_tokensB(10) split out)._load_toolnow validates the JSON envelope, per-entry types, three match modes with two different duplicate policies, the default response, and passthrough executable resolution in one 59-line body (lines 100-158);dispatch(lines 203-227) now inlines the whole subset scan. Extract the per-entry work —_load_response_entry(entry, index, fixture_path) -> (mode, key_or_tokens, CommandResponse)— out of thefor index, entryloop, and lift lines 214-222 intodef _match_subset(state: ToolState, argv: list[str]) -> CommandResponse | None. Both restore the B band without changing behavior. (CC 10-20 outside a hot module is the 🟡 anchor; protected_mock is not in the orchestrator/checker/sandbox hot set, so this does not reach 🟠.) - [Axis 2] Fixture entries are hand-parsed
Anydicts with noextra="forbid": a typo'dmatch_modekey silently degrades asubsetrule to exact matching (so every argv variant falls through todefault), and a non-hashablematch_moderaisesTypeErrorat the set-membership test instead of the descriptiveValueErrorone line below (src/coder_eval/protected_mock/server.py:124) —_load_toolreads the per-response entry off untyped JSON, somatch_modeisAnyand nothing rejects unknown sibling keys:
124: match_mode = entry.get("match_mode", "exact")
125: if match_mode not in {"exact", "normalized", "subset"}:
126: raise ValueError(f"fixture {fixture_path} response {index}.match_mode must be exact, normalized, or subset")
127: if match_mode == "subset":
...
134: destination = responses if match_mode == "exact" else normalized_responses
135: command_key = key if match_mode == "exact" else _normalized_argv(argv)Two verified holes (run against pr-93 with the project venv):
- Unknown key silently dropped. A fixture entry
{"argv": ["rpa","get-errors"], "match_modes": "subset", ...}(typo'd key) loads without error and registers as an exact rule —_load_toolreturnedexact keys: [('rpa','get-errors')] | subset rules: []. The author asked for subset matching and silently got exact, so every argv variant resolves to the fixturedefaultinstead of the intended canned response. This is exactly theextra="forbid"convention the sibling model already follows (src/coder_eval/models/sandbox.py:417,model_config = ConfigDict(extra="forbid")onProtectedMockConfig); the fixture schema — the half that actually selects the response — has no equivalent. - Non-hashable value crashes instead of failing cleanly.
{"match_mode": ["subset"]}makes line 125's set-membership test raiseTypeError: unhashable type: 'list'(verified), so the descriptiveValueErroron line 126 is never reached and mockd dies with a bare traceback.
Fix: parse the fixture with a Pydantic model mirroring ProtectedMockConfig — model_config = ConfigDict(extra="forbid"), argv: list[str], match_mode: Literal["exact", "normalized", "subset"] = "exact", exit_code: int = Field(0, ge=0, le=255) — which subsumes the hand-rolled isinstance ladder in _load_tool/_response, makes the closed set exhaustiveness-checkable by pyright instead of re-tested as magic strings on lines 125/127/134/135, and turns both cases above into a loud validation error. Candidate for a CEnnn rule: "a closed string set tested by x not in {...} literal must be a Literal/enum".
3. [Axis 3] running_mock_server's success path and new Popen-failure cleanup guard are untested (runtime.py at 76.47%; the repo-wide 80% --cov-fail-under is not a per-module gate) (src/coder_eval/protected_mock/runtime.py:61) — The two new tests cover only the two failure exits. Reproduced module coverage is 76.47% with missing lines 28-29, 31, 38-39, 61-63, 76, 84, 90-92. Three of those are this PR's own new/restructured code: (a) 61-63, the new except OSError: / stderr_path.unlink(missing_ok=True) / raise guard — the temp-file-leak protection this PR added never executes, so a regression dropping the unlink ships green; (b) 76 and 84, break on socket_path.exists() and the yield — i.e. no test ever takes the happy path through the rewritten function, even though the PR moved subprocess.Popen inside a with tempfile.NamedTemporaryFile(prefix="coder-eval-mockd-", suffix=".stderr", delete=False) as stderr_sink: block that closes the parent handle while the child still holds the inherited fd; (c) 28-29 and 31, _server_stderr_suffix's except OSError: return "" and if not text: return "" — a silent child would otherwise yield a dangling ; server stderr (tail): suffix and nothing asserts it does not. Add a success-path test (stub child that touches the socket path; assert the context manager yields and the recorded stderr temp file is gone after exit), a monkeypatch making Popen raise OSError (assert it propagates and the temp file is unlinked), and a silent-child case asserting the RuntimeError message carries no stderr suffix.
4. [Axis 5] Stale "exact-command fixture service" claim in server.py docstring and sibling doc/model sites after subset mode (src/coder_eval/protected_mock/server.py:1) — The PR correctly removed the sentence It never performs subset or substring matching. from docs/TASK_DEFINITION_GUIDE.md and rewrote that section accurately, but the same guarantee is restated verbatim elsewhere and was not rippled (Technique 1). The in-scope offender is the enforcement module's own docstring, server.py:1: """mockd: exact-command fixture service running as the private mock UID.""" — with subset mode this is now false, not merely imprecise (pre-PR it was a defensible shorthand because normalized still selected from a finite command map, which is exactly what _normalized_argv's surviving docstring at server.py:79 still says: """Canonical finite-command key: flag form/order agnostic, never subset matching."""). Three out-of-scope siblings carry the same stale claim and should be updated in the same commit: src/coder_eval/models/sandbox.py:413 (The fixture schema maps exact argv lists to bounded stdout/stderr/exit-code), src/coder_eval/models/sandbox.py:420 (fixture: str = Field(description="Path to the protected exact-command response fixture") — this one is a user-facing schema description, the surface CE030 doc-schema parity governs), and docs/DOCKER_ISOLATION.md:308 (| Protected fixture service | container, UID/GID \2100:2100`, exact-command Unix RPC |, a row in the isolation threat-model table). src/coder_eval/protected_mock/client.py:1saysexact-command mock servicetoo but is arguably harmless since the client does no matching. This matters beyond cosmetics becauseprotected_mockis a security boundary: a reader of the threat-model row or thefixturefield description concludes an agent must reproduce a full argv to pull a canned answer, and therefore under-estimates how cheaply an agent can probe the fixture one token at a time under subset matching. Fix: rewordserver.py:1to"""mockd: fixture-backed CLI service running as the private mock UID."""and update the three sibling sites to say the fixture maps argv *rules* (exact / normalized / subset) rather than exact argv lists. 5. **[Axis 6] Pre-existing: unguarded post-killprocess.wait(timeout=5)in thefinally can skip both unlinks and mask the socket-timeout RuntimeError** (src/coder_eval/protected_mock/runtime.py:92`) — Lines 85-96 read:
finally:
if process.poll() is None:
process.terminate()
try:
process.wait(timeout=5)
except subprocess.TimeoutExpired:
process.kill()
process.wait(timeout=5) # line 92 - unguarded
with contextlib.suppress(OSError):
os.unlink(socket_path)
with contextlib.suppress(OSError):
os.unlink(stderr_path)The second process.wait(timeout=5) at line 92 is not wrapped. If it raises subprocess.TimeoutExpired (child wedged in uninterruptible sleep), that exception propagates out of the finally and (a) skips the socket unlink at line 94 and the stderr unlink at line 96, leaking /run/coder-eval/uip.sock and the temp file, and (b) replaces the in-flight exception — the startup RuntimeError raised at line 71 or 81 that carries the stderr tail, which is the entire diagnostic this PR adds. The diff widened this window (timeout=3 → timeout=5 on both waits). Fix: wrap the kill-and-reap in with contextlib.suppress(subprocess.TimeoutExpired): so cleanup always reaches lines 93-96. Coverage corroborates that this path is unexercised (lines 90-92 uncovered). n/a
6. [Axis 7] Subset rules silently shadow passthrough_argv_prefixes: precedence is neither documented nor tested (docs/TASK_DEFINITION_GUIDE.md:584) — Line 584 presents what reads as the complete resolution order — exact and normalized matches always take precedence over subset scanning — but dispatch (server.py:211-227) resolves in the order exact → normalized → subset → if response is not None: return response → if any(tuple(argv[: len(prefix)]) == prefix for prefix in state.passthrough_prefixes): return self._passthrough(...). Subset therefore outranks passthrough, which makes the still-unchanged sentence at line 588 ("mockd invokes the real tool only when argv begins with one of these typed prefixes") false in a newly reachable way. Concretely: with passthrough_argv_prefixes: [[docsai, ask]] and a fixture rule {"argv": ["docsai"], "match_mode": "subset"} (a natural way to canned-answer the non-ask docsai subcommands), the invocation uip docsai ask "..." has docsai in its token set, so the subset rule wins at server.py:220 and the real tool is never invoked. Before this PR the shadowing risk was theoretical — a passthrough call carries free-form text, so it essentially never produced an exact/normalized full-argv key collision; token-set matching makes a one-token rule sufficient. Extend line 584 to state the full chain (exact → normalized → subset → passthrough prefix → default) and warn that a subset rule whose tokens are contained in a passthrough invocation disables that passthrough; alternatively move the passthrough-prefix check ahead of the subset scan in dispatch so that passthrough remains authoritative for its declared prefixes.
Nits
- [Axis 1] Redundant
not argvdisjunct in the subset empty-argv guard (src/coder_eval/protected_mock/server.py:129) — Line 129 isif not argv or not rule_tokens:.rule_tokenson line 128 istuple(_expand_argv_tokens(argv)), and_expand_argv_tokens([]) == [](verified at the PR head), sonot argvstrictly impliesnot rule_tokensand can never independently trip. Drop it toif not rule_tokens:— the message on line 130 already covers both cases and the test at lines 275-284 exercises them both through the single surviving condition. - [Axis 1] Startup-timeout message prints the same timeout number twice (waited == deadline) (
src/coder_eval/protected_mock/runtime.py:79) — Lines 79-83 add three lines of machinery for a value that is redundant:
waited = time.monotonic() - started
deadline_note = f"within {waited:.1f}s (deadline {STARTUP_TIMEOUT_SECONDS}s)"The else clause runs only when the while time.monotonic() < deadline on line 69 falls through, i.e. waited >= STARTUP_TIMEOUT_SECONDS, and the poll granularity is time.sleep(0.02) (line 77), so at one decimal place the rendered message is always did not create its socket within 30.0s (deadline 30.0s) (or within 0.3s (deadline 0.3s) under the test's monkeypatch at tests/test_protected_mock.py:346). Drop started/waited/deadline_note and inline f"protected mockd did not create its socket within {STARTUP_TIMEOUT_SECONDS}s".
3. [Axis 1] The subset ordering contract is restated verbatim as a comment in two places plus the docs (src/coder_eval/protected_mock/server.py:114) — The same sentence appears three times. server.py:114-116: "Ordered on purpose: subset rules are scanned in fixture-file order and the first match wins..."; server.py:215-217: "Finite matches take precedence; subset rules scan in fixture-file order and the first whose tokens all appear in the invocation's normalized token set wins."; docs/TASK_DEFINITION_GUIDE.md:584: "Subset rules are evaluated in fixture-file order and the first match wins". The second comment narrates the six lines directly beneath it (a for ... if all(...) ... break). Keep the load-bearing half of the first comment (why duplicates are legal for subset but rejected for the finite modes, which is not obvious from the code) and delete the dispatch-side restatement.
4. [Axis 2] subset_responses uses an anonymous positional tuple pair whose token type (tuple[str, ...]) misrepresents its set semantics (src/coder_eval/protected_mock/server.py:34) — The new ToolState field is 34: subset_responses: list[tuple[tuple[str, ...], CommandResponse]], unpacked positionally at 219: for rule_tokens, candidate in state.subset_responses:. Two type-expressiveness problems: (a) the pair is anonymous, so the reader must jump to the producer (line 131) to learn which slot is which; (b) the rule-token type is the same tuple[str, ...] used as the order-significant key of responses/normalized_responses on the same dataclass, but matching is pure set membership — 220: if all(token in invocation_tokens for token in rule_tokens): against 218: invocation_tokens = set(_expand_argv_tokens(argv)) — so order and duplicates inside a subset rule are meaningless. One type carrying two different semantics in one dataclass invites a future reader to assume the subset rule is ordered. Fix: @dataclass(frozen=True) class SubsetRule: tokens: frozenset[str]; response: CommandResponse and subset_responses: list[SubsetRule]; the frozenset makes the set semantics type-visible and drops the redundant duplicate scan. Same theme, secondary: 46: def _expand_argv_tokens(argv: list[str]) -> list[str]: returns a mutable list that all three call sites (lines 81, 128, 218) immediately freeze into a tuple/set — Sequence[str] -> tuple[str, ...] states the contract.
5. [Axis 2] Test helper returns a bare MagicMock used as self for the unbound ProtectedMockServer.dispatch, so any attribute the server later reads is auto-vivified instead of failing (tests/test_protected_mock.py:67) — The new helper is untyped against the real class:
67: def _fake_server(tools: dict[str, ToolState]) -> MagicMock:
68: fake = MagicMock()
69: fake.tools = tools
70: fake.budget_lock = threading.Lock()
71: fake.passthrough_lock = threading.Lock()
72: return fakeand is passed as self to the unbound method in the four new subset tests (lines 218, 237, 259, 272), e.g. ProtectedMockServer.dispatch(fake_server, "uip", ["rpa", "get-errors"]). With no spec=, the mock satisfies any attribute access: if dispatch later reads a new self.<lock> (a MagicMock supports the context-manager protocol, so with self.new_lock: succeeds), or a new self.<state> field, these tests keep passing green while the real ProtectedMockServer.__init__ — which they never execute — is untested. This is the shared review criterion 'test mocks match real SDK shape' (use Mock(spec=RealType)). Fix: MagicMock(spec_set=ProtectedMockServer), so an unconfigured attribute raises AttributeError, or construct a real ProtectedMockServer against a tmp_path socket.
6. [Axis 3] The new three-mode match_mode rejection message is uncovered (server.py:126) (src/coder_eval/protected_mock/server.py:126) — This PR rewrote the validator from must be exact or normalized to raise ValueError(f"fixture {fixture_path} response {index}.match_mode must be exact, normalized, or subset"), and line 126 is in the missing-lines list — no test loads a fixture with an unknown match_mode (there is no pytest.raises(..., match="match_mode must be") anywhere in tests/test_protected_mock.py). Add a one-line load test with "match_mode": "prefix" asserting the message names all three modes, so the accepted mode set and its user-facing error stay in sync when a fourth mode is added.
7. [Axis 3] _stub_mockd_child monkeypatches the stdlib subprocess/tempfile module globals, and one startup test weakens its cleanup assertion behind a win32 guard (tests/test_protected_mock.py:313) — Two nits in the new startup-test helper. (1) monkeypatch.setattr("coder_eval.protected_mock.runtime.subprocess.Popen", fake_popen) and monkeypatch.setattr("coder_eval.protected_mock.runtime.tempfile.NamedTemporaryFile", recording_named_temp) (lines 313-314) resolve to the stdlib subprocess and tempfile modules — runtime.py does import subprocess / import tempfile, not from ... import ... — so they replace the attribute process-wide for the test's duration. fake_popen ignores the argv it is handed and always runs the stub script, and created[0] would be the wrong file if anything else opened a NamedTemporaryFile first. Patch a module-local seam instead, or assert on the entry whose name carries the coder-eval-mockd- prefix rather than created[0]. (2) if sys.platform != "win32": at line 355 makes assert created and not created[0].exists() conditional in the timeout test while the sibling test_mockd_startup_exit_reports_child_stderr asserts it unconditionally at line 333 — on a component that is Linux-only by construction (SO_PEERCRED at server.py:285-287, chown/geteuid at server.py:326-330). Drop the guard so both tests pin the same contract.
8. [Axis 6] Readiness probe tests socket file existence, not connectability, so it can yield before mockd chowns/chmods the socket (src/coder_eval/protected_mock/runtime.py:75) — The startup poll breaks on if socket_path.exists(): (line 75). In server.py::serve, the socket is created by ProtectedMockServer(str(socket_path), tools) (line 331, socketserver binds+listens in __init__), and only afterwards does the try: body run chown(socket_path, geteuid(), MOCK_RPC_GID) and socket_path.chmod(0o660) (server.py:330-333). Between bind and chmod the socket exists with default (root-owned, non-uip-rpc) permissions, so running_mock_server can yield a socket the agent uid cannot connect to — surfacing as client exit 125 rather than a loud harness error. The window is only two syscalls wide and the parent polls every 20 ms, so this is very unlikely in practice, but the PR is titled "harden mockd startup" and leaves the readiness contract as "file exists" instead of "a connect() succeeds". Fix: replace the exists() probe with a best-effort socket.connect(SOCKET_PATH) attempt, or have mockd touch a separate ready-marker after chmod. n/a
9. [Axis 6] New captured-stderr file is an unbounded, agent-influenceable on-disk sink with no size cap (src/coder_eval/protected_mock/runtime.py:46) — Line 46 redirects mockd's stderr into tempfile.NamedTemporaryFile(prefix="coder-eval-mockd-", suffix=".stderr", delete=False) in the default temp dir, and the justification comment at lines 41-45 reasons only about confidentiality ("created 0600 and owned by the spawning process ... so the agent uid cannot read it") — never about size. ProtectedMockServer inherits socketserver.ThreadingMixIn, whose process_request_thread calls handle_error() (a traceback.print_exc() to stderr) on any handler exception; ProtectedMockHandler._write's self.wfile.write(payload) (server.py:308) raises BrokenPipeError whenever a peer disconnects before reading. The agent holds shell access and can open sockets directly (the socket is 0660 uip-rpc, and handle()'s early-return paths do not consume the max_requests budget), so it can drive an unbounded traceback stream into a file that is never rotated or truncated for the life of the run. Fix: cap the sink (e.g. periodically truncate, or use a fixed-size ring/RLIMIT_FSIZE on the child) and record the size reasoning in the comment alongside the 0600 reasoning. n/a
10. [Axis 7] Subset load error says argv "must be non-empty" when argv is non-empty but normalizes to zero tokens (src/coder_eval/protected_mock/server.py:130) — Lines 129-130 collapse two distinct failures into one message: if not argv or not rule_tokens: raise ValueError(f"fixture {fixture_path} response {index}.argv must be non-empty for subset matching"). The not rule_tokens branch fires for a non-empty argv that reduces to nothing after noise stripping — the PR's own test hits it with {"argv": ["--output", "json"], "match_mode": "subset"} (tests/test_protected_mock.py:280-285). A task author reading "argv must be non-empty" while looking at a two-element argv has no path to the real cause. Split the branches, e.g. keep the current text for not argv and emit ...argv {argv!r} contains only ignored tokens (--output and its value are stripped) and cannot be used for subset matching for not rule_tokens.
What's Missing
Daily/nightly:
- 🟠 The fixture schema gained a new
match_modevalue butPROTOCOL_VERSIONstays1and no image capability label was added —protected_mock/server.pyis baked into the container image (docker/Dockerfile installs the package), and_preflight_image_versiononly warns on host/image drift (isolation/docker_runner.py:257), so the first nightly task whose fixture usesmatch_mode: subsetagainst a pre-PR image dies at mockd load withmust be exact or normalizedand fails the whole task with only a log warning. The repo already has the right pattern (org.coder-eval.agent-isolation=uid-gid-v1, hard-fail at docker_runner.py:290) — bump a fixture/capability signal, or at minimum state the image-rebuild ordering requirement for the nightly. (trigger: src/coder_eval/protected_mock/server.py) - 🟡 The 5s→30s startup deadline sits on the production per-task container path (
cli/run_task_internal_command.py:237), so a mockd that never binds now burns 25s more per task, multiplied across nightly tasks × replicates × parallel workers; the value is a module-level literal with no env/config override and the PR states no expected nightly impact. (trigger: src/coder_eval/protected_mock/runtime.py)
Tests:
- 🟠 No test drives the new mode through a real server: all four subset tests call the unbound
ProtectedMockServer.dispatchwith a bareMagicMockself, so__init__, the socket handler, the client round-trip, and thecalls.jsonlrecord are unexercised forsubset— and there is zero in-tree consumer (no fixture, task YAML, ortests/test_docker_identity_isolation.pycase usesmatch_mode: subset), so the mode ships unit-tested only. (trigger: tests/test_protected_mock.py) (restates: Axis 2: Test helper returns a bare MagicMock used asselffor the unbound ProtectedMockServer.dispatch) - 🟠 The flag-bearing subset rule — the shape the guide advertises and the shape
normalizedmode is authored with — has no test, so the position-free flag/value decoupling (--job-id 42matching--job-id 99 --tag 42) is unpinned; add the negative case plus the--job-id=inline form (the still-uncovered58->50branch). (trigger: src/coder_eval/protected_mock/server.py) (restates: Axis 3: New subset match mode has no test for a rule containing a flag+value) - 🟡 No test covers subset-vs-passthrough precedence, even though the new subset scan (server.py:214-222) now runs before the
passthrough_argv_prefixescheck and a one-token subset rule can silently disable a declared passthrough prefix. (trigger: tests/test_protected_mock.py) (restates: Axis 7: Subset rules silently shadow passthrough_argv_prefixes) - 🟡 The rewritten
running_mock_serverhas no happy-path test (thebreakat :76 andyieldat :84 never execute) and no test for the newexcept OSErrortemp-file cleanup guard at :61-63 — deleting theunlinkleaves the suite green, so this PR's own leak protection ships unverified. (trigger: src/coder_eval/protected_mock/runtime.py) (restates: Axis 3: running_mock_server's success path and new Popen-failure cleanup guard are untested)
Downstream consumers:
- 🟡 The noise-flag skip rewrite silently changed the existing
normalizedmode: a valueless--outputno longer swallows the following token when that token is flag-shaped, so_normalized_argvkeys move for both fixture rules and invocations (e.g.deploy --output --delete-allused to key as('deploy',), now('--delete-all','deploy')). Existing out-of-tree normalized fixtures can flip between a canned answer and the exit-2 default; the guide'snormalizedbullet was rewritten without noting the change, and nothing audits shipped fixtures. (trigger: src/coder_eval/protected_mock/server.py) - 🔵 Grading consumers of
cli_mocks/calls.jsonlweren't revisited: a broad subset rule converts whole argv families from the exit-2 default into canned successes (changing the recordedexitthecli_calledcriterion reports and the trajectory artifact/judge criteria grade), yet the guide adds no authoring guidance on how narrow a subset rule should be, especially next to negative-guard tasks. (trigger: docs/TASK_DEFINITION_GUIDE.md)
Parallel paths:
- 🟡
STARTUP_TIMEOUT_SECONDSwent 5s→30s with a loaded-box rationale (parallel workers, cold caches), but the sibling budget on the same RPC path —protocol.py::CLIENT_TIMEOUT_SECONDS = 5.0, applied to connect+send+recv inclient.py:67— was left at the old 5s; it is also 12× belowserver.py::PASSTHROUGH_TIMEOUT_SECONDS = 60, so any passthrough call that actually shells out hands the agent exit 125 while mockd is still running the real tool. (trigger: src/coder_eval/protected_mock/runtime.py) - 🟡 Fixture content is still validated in exactly one place — container-side
_load_toolat mockd startup. The host-sideProtectedMockConfig(models/sandbox.py:409) validatestool/prefixes but never parses the fixture, so the third match mode widens the authoring surface whose typos surface only as a task-killing mockd startup failure inside the container instead of atcoder-eval plan. (trigger: src/coder_eval/protected_mock/server.py) (restates: Axis 2: Fixture entries are hand-parsed Any dicts with no extra="forbid") - 🔵 The "exact-command" guarantee was correctly rewritten in the task guide but not rippled to the four sibling statements of the same claim:
server.py:1,models/sandbox.py:413and:420(the user-facingfixturefield description),client.py:1, and thedocs/DOCKER_ISOLATION.md:308threat-model row. (trigger: src/coder_eval/protected_mock/server.py) (restates: Axis 5: Stale "exact-command fixture service" claim in server.py docstring and sibling doc/model sites)
Display & mapping dicts:
- 🟡 The closed
match_modeset has no single source of truth: it is restated as a set literal (server.py:125), a user-facing error string (:126), three branch comparisons (:127/:134/:135), and prose in the guide, with noLiteral/enum onProtectedMockConfig— adding a fourth mode needs five coordinated edits and pyright cannot check exhaustiveness. (trigger: src/coder_eval/protected_mock/server.py) (restates: Axis 2: Fixture entries are hand-parsed Any dicts with no extra="forbid") - 🔵 The fixture JSON example (guide lines 561-577) still shows only the exact form — the new
subsetmode gets prose but no example entry and no worked illustration of the exact → normalized → subset → passthrough → default resolution chain, so a task author copying the example has nothing to adapt. (trigger: docs/TASK_DEFINITION_GUIDE.md)
Harness & Lint Improvements
Static checks (lint / type):
- [ce-lint] CE032 — user-authored documents must be parsed through a pydantic model with
extra="forbid". New ruletests/lint/rules/ce032_config_documents_pydantic_parsed.py, class added toALL_RULESintests/lint/runner.py, cases intests/test_custom_lint.py. Pattern forbidden: ajson.loads(...)/yaml.safe_load(...)result that is hand-validated in the same function (>=2isinstance(...)guards or >=2.get(...)reads on the parsed object) instead of flowing intoSomeModel.model_validate(...)/TypeAdapter(...).validate_python(...). Scope with the CE009 hard-coded-path-filter template (src/coder_eval/protected_mock/**,orchestration/task_loader.py,criteria/**) so internal-artifact readers (run.json, package.json, resume fingerprints — 20json.loadssites repo-wide) stay out of scope;# noqa: CE032is the escape. Baseline inside the scoped paths at PR head: exactly one violation,protected_mock/server.py::_load_tool. The fix it forces (match_mode: Literal["exact","normalized","subset"],exit_code: int = Field(0, ge=0, le=255),ConfigDict(extra="forbid")— mirroring the siblingProtectedMockConfigatmodels/sandbox.py:417) also makes the closed mode set exhaustiveness-checkable by pyright instead of a magic-string set literal repeated at server.py:125/127/134/135. Note pyright cannot reach this today on its own:json.loadsis declared-> Any(explicit, not Unknown), so evenstrictmode stays silent — the rule is the only static lever. Prevents: A2 medium (typo'dmatch_modeskey silently degrades a subset rule to exact matching so every argv variant falls through todefault; non-hashablematch_moderaisesTypeErrorat the set-membership test before the descriptiveValueErroron line 126); A3 low (uncovered magic-string mode-rejection message — a Literal removes the hand-written message entirely); A7 low (the misleading "argv must be non-empty" message becomes a typed validator with the real cause). - [ruff] Enable a cyclomatic-complexity gate — there is none today.
[tool.ruff.lint] selectcurrently carriesPLR0915/PLR0912(statements/branches) but noC90, so a function can grow arbitrarily branchy without trippingmake check. Add"C90"toselectand[tool.ruff.lint.mccabe] max-complexity = 10, applied diff-scoped in the PR-checks job (ruff check --select C901 $(git diff --name-only origin/$BASE...HEAD -- '*.py')) rather than repo-wide: measured onsrc/, threshold 10 => 57 pre-existing violations, 12 => 28, 15 => 14, 20 => 4, so a repo-wide flip at 10 would need a mass# noqa: C901debt sweep, while diff-scoping holds only new/edited functions to the bar — the same "gate NEW growth, track existing offenders" philosophy the PLR0915 comment already states. ruff's mccabe scores_load_tool9 -> 11 (fails at 10) anddispatch6 -> 9 (passes), so pair the gate with the existing branch cap and keep review for the rest. Prevents: A1 medium (_load_toolradon C(17) -> C(20) / ruff 9 -> 11;dispatchB(7) -> C(12)) — the_load_response_entryand_match_subsetextractions would have been forced at commit time instead of surfaced at review. - [ce-lint] CE033 — teardown calls in a
finallyblock must not be able to raise. New ruletests/lint/rules/ce033_no_raising_call_in_finally.py+ALL_RULESwiring. Flag calls whose attribute name is in{wait, kill, terminate, communicate, unlink, remove, close, rmtree}that appear anywhere in afinally:body — including inside the body of anexcepthandler nested in thatfinally(the exact shape atprotected_mock/runtime.py:92, where the post-killprocess.wait(timeout=5)sits in anexcept subprocess.TimeoutExpired:handler and is therefore unguarded) — unless wrapped incontextlib.suppress(...)or atry/exceptwhose body encloses them. Measured baseline over the full PR-headsrc/tree with this exact predicate: 3 sites (runtime.py:87process.terminate(),runtime.py:92the unguarded reap,server.py:335socket_path.unlink(missing_ok=True)) — cheap to adopt. Prevents: A6 medium (aTimeoutExpiredescaping thefinallyskips bothos.unlinkcalls, leaking/run/coder-eval/uip.sockplus the stderr temp file, and replaces the in-flight socket-timeoutRuntimeErrorthat carries the stderr tail this PR added). - [ce-lint] CE034 — a spec-less mock may not be used as
self. New ruletests/lint/rules/ce034_no_specless_mock_as_self.py+ALL_RULESwiring; scopetests/**. Within a module, track names bound toMock(...)/MagicMock(...)constructed withoutspec=/spec_set=, then flag any call of the formPascalCaseName.method(<that name>, ...)(an unbound-method call taking the mock as the receiver). RequireMagicMock(spec_set=RealType)or a real instance. Measured baseline repo-wide: 0 matches (123 spec-lessMagicMock()exist, but none is currently used as an unboundself), so this rule lands clean and stays narrow — a blanket "always passspec=" rule would be far too noisy here. Prevents: A2 low (_fake_serverat tests/test_protected_mock.py:67 is a bareMagicMockpassed asselftoProtectedMockServer.dispatchat lines 218/237/259/272; any futureself.<new_lock>orself.<new_state>the server reads is auto-vivified — aMagicMockeven satisfies the context-manager protocol — so the four subset tests stay green against a server whose real__init__they never execute). - [ce-lint] CE035 — no inline platform branching in tests. New rule
tests/lint/rules/ce035_no_inline_platform_branch_in_tests.py+ALL_RULESwiring; scopetests/**. Flagif sys.platform .../if os.name ...conditions inside atest_*function or a fixture body; platform gating must be@pytest.mark.skipif(sys.platform == ..., reason=...)so the whole test is skipped rather than an assertion silently dropped. Measured baseline repo-wide: 0 inline branches vs 7 existingskipif(sys.platform ...)decorators — the convention already exists and is unenforced. Prevents: A3 low (tests/test_protected_mock.py:355 wrapsassert created and not created[0].exists()inif sys.platform != "win32":, weakening the timeout test's cleanup assertion relative to its sibling at line 333 — on a component that is Linux-only by construction:SO_PEERCREDat server.py:285-287,chown/geteuidat server.py:326-330). - [ce-lint] CE036 —
monkeypatch.setattrtargets must not traverse an imported stdlib module. New ruletests/lint/rules/ce036_no_stdlib_traversal_monkeypatch.py+ALL_RULESwiring; scopetests/**. Flag string targets matching^coder_eval\..*\.(subprocess|tempfile|os|shutil|socket|time)\.— because the target module doesimport subprocess/import tempfile, the attribute being replaced is the stdlib module global, i.e. a process-wide patch for the test's duration, not a module-local seam. Require patching a module-local indirection (a wrapper function/attribute on the module under test) instead. Honest adoption cost: 6 pre-existing sites (tests/test_docker_runner_mounts.py:527,541,626,643,tests/test_docker_runner_container_death.py:193,tests/test_codex_agent.py:1813) must each grow a seam or a# noqa: CE036. Prevents: A3 low (the new_stub_mockd_childhelper at tests/test_protected_mock.py:313-314 patchesruntime.subprocess.Popenandruntime.tempfile.NamedTemporaryFileprocess-wide;fake_popenignores the argv it is handed and the assertions indexcreated[0], which is the wrong file if anything else opens aNamedTemporaryFilefirst). - [ce-lint] CE037 — no anonymous heterogeneous tuple in a
@dataclass/BaseModelfield annotation. New ruletests/lint/rules/ce037_named_types_for_field_tuples.py+ALL_RULESwiring; scopesrc/coder_eval/**. Flag anAnnAssignin the body of a class that is@dataclass-decorated or directly extendsBaseModelwhose annotation contains a nestedtuple[...]with >=2 differing element types; require a named frozen dataclass /NamedTuple. Measured baseline repo-wide: 0 (the two tuple-typed fields that exist —BaseAgentConfig._merge_exclusive_groupsandJudgeContext.dialog— are homogeneous and stay clean). Locals and function parameters are deliberately out of scope to keep noise at zero. Prevents: A2 low (ToolState.subset_responses: list[tuple[tuple[str, ...], CommandResponse]]at server.py:34 — an anonymous pair unpacked positionally at line 219, whose token slot reuses the sametuple[str, ...]type as the order-significant keys ofresponses/normalized_responseseven though subset matching is pure set membership; a namedSubsetRule(tokens: frozenset[str], response: CommandResponse)makes the set semantics type-visible). - [ce-lint] CE038 — doc-surface parity for nested user-facing config models and their closed sets. Wire as a dedicated
@pytest.mark.linttest class alongside CE026-CE031 (these reason over Markdown + the whole tree, so they are notBaseRules), extendingtests/lint/doc_schema_parity.py. Two assertions: (1) widen CE030's model set beyondTaskDefinition/RunLimits/Dataset/SimulationConfigto config models nested underSandboxConfigthat users author directly (ProtectedMockConfig, and the fixture-entry model CE032 forces into existence) — a verifier check confirmed CE030's own header explicitly excludes nested models today, so nothing currently governs this surface; (2) closed-set + vocabulary parity: every member of a user-facingLiteral(the fixturematch_mode) must appear as inline code indocs/TASK_DEFINITION_GUIDE.md, and the phraseexact-commandmay not appear undersrc/coder_eval/protected_mock/**,models/sandbox.py, ordocs/DOCKER_ISOLATION.mdwhile that Literal has more than one member. Prevents: A5 medium (the stale "exact-command fixture service" claim left inprotected_mock/server.py:1,models/sandbox.py:413, the user-facingfixtureField description atmodels/sandbox.py:420, the threat-model row atdocs/DOCKER_ISOLATION.md:308, andprotected_mock/client.py:1— the guide was updated correctly, the five sibling surfaces were not, and no existing lint rule covers any of them).
Harness improvements (not statically reachable):
- Diff-scoped coverage gate. Add
make cov-diff(and a PR-checks job) that runscoverage xmlanddiff-cover --compare-branch=origin/$BASE --fail-under=90, so changed lines must be exercised even when the repo-wide number is healthy. The existing gate is--cov-fail-under=80across the wholecoder_evalpackage (Makefile:57 and .github/workflows/pr-checks.yml:144), which a 56-statement module sitting at 76.47% passes without a murmur. Why not static: Line-level execution data only exists after the suite runs; no AST or grep pass can tell a reachable branch from an exercised one. Prevents: A3 medium (runtime.py's newexcept OSError:/stderr_path.unlink(missing_ok=True)/raiseguard at 61-63, thebreakat 76, theyieldat 84 — i.e. the entire happy path of the rewrittenrunning_mock_server— plus_server_stderr_suffix's 28-29/31 early returns, all uncovered); A3 low (the rewritten three-modematch_moderejection message at server.py:126). - Mutation smoke on changed lines. Add an advisory PR job running
mutmut/cosmic-ray(or a cheap delete-a-statement harness) restricted to the diff's files, reporting surviving mutants as a comment rather than a hard gate. The verifier demonstrated the precise gap this closes: deletingstderr_path.unlink(missing_ok=True)from the new Popen-failure guard leaves the whole suite green (16 passed), so the temp-file-leak protection this PR added ships unverified. Why not static: "Covered but unasserted" is invisible to both lint and coverage — it is only observable by perturbing the code and re-running the suite. Prevents: A3 medium (new safety code with no assertion behind it); generalizes to every future teardown/cleanup guard added in afinally. - Matcher contract/property tests for the fixture DSL. Add a hypothesis (or table-driven) suite over
_expand_argv_tokens/_load_tool/dispatchpinning the invariants the guide advertises: (a) permutation invariance — every ordering of a subset rule's argv loads to the same token set; (b) no silent narrowing — a rule's loaded token count equals its non-noise token count, which fails loudly for["--output","rpa","get-errors"] -> ('get-errors',); (c) flag/value semantics — a--job-id 42rule against--job-id 99 --tag 42, plus the--job-id=42inline form and the still-uncovered non-noise empty-value branch (58->50). Adopt the general habit: every invariance claim written in the docs ("regardless of order") gets a metamorphic test. Why not static: The defect is a semantic asymmetry — one helper is correct for invocation-side argv and wrong for rule-side argv — so it is only visible by executing both directions and comparing results; no syntactic pattern distinguishes the two call sites. Prevents: A8 high (rule argv run through the invocation-side noise-flag scanner: a bare--outputin a subset rule silently eats the following rule token, widening the rule to match any invocation containing the survivor); A3 high (no subset test uses a flag-bearing rule, so the position-free flag/value decoupling is entirely unpinned). - Resolution-order golden test. One table-driven test over a single fixture that declares all five tiers, asserting which tier each invocation resolves to across exact -> normalized -> subset -> passthrough prefix -> default, and including the shadowing case explicitly (
passthrough_argv_prefixes: [["docsai","ask"]]plus a subset rule["docsai"], whereuip docsai ask "..."currently never reaches the real tool). Pair it with the guide stating the full chain at docs/TASK_DEFINITION_GUIDE.md:584 rather than only the exact/normalized-over-subset half. Why not static: Precedence is emergent from statement order insidedispatch(server.py:211-227); lint cannot distinguish an intended ordering from an accidental one, and no rule can know that a fixture's subset tokens are contained in a declared passthrough prefix without executing the matcher. Prevents: A7 medium (subset rules silently outrankpassthrough_argv_prefixes; neither documented nor tested, and no in-tree fixture combines the two, so today it is a latent config-authoring footgun). - Sidecar liveness contract for mockd. On exiting
running_mock_server, consultprocess.poll()before unlinking: when the service died during the body, log at ERROR with_server_stderr_suffix(stderr_path)(and surface a harness-level failure distinct from an agent failure) instead of deleting the capture unread at runtime.py:96. Add an integration test that SIGKILLs mockd mid-body and asserts the operator-visible signal. Worth noting while doing it:socketserver.handle_errorwrites every per-request handler traceback (e.g. theBrokenPipeErrorfromProtectedMockHandler._write) into that same file, so even when mockd survives, all handler diagnostics are discarded — the capture this PR added is consumable only on the startup path. Why not static: "Was this diagnostic read before the file was deleted on every teardown path?" is cross-branch data flow through ayieldboundary, and proving the mis-attribution (agent-side exit 125 fromclient.py::invokerather than a harness error) needs a live child process and run-level state. Prevents: A6 high (post-startup mockd exit never checked and captured stderr unlinked unread — a mid-run death scores as an agent failure with no harness signal). - Readiness = capability probe, not artifact existence. Replace the
socket_path.exists()poll at runtime.py:75 with a best-effortsocket.connect(SOCKET_PATH)(or have mockd write a ready-marker afterchown/chmod), and add a stub-server test that binds and delays the chmod to prove the probe waits for connectability. This test doubles as the missing happy-path exercise ofrunning_mock_server(yield reached, stderr temp file unlinked on exit). Adopt as a harness convention for every spawned sidecar. Why not static: The bind-before-chmod window is a runtime ordering property of a different process (socketserverbinds in__init__at server.py:331;chown/chmodrun afterwards at 330-333) — no single-file AST pass can see across that boundary. Prevents: A6 low (yielding a socket the agent uid cannot yet connect to, surfacing as client exit 125 rather than a loud harness error); also closes part of A3 medium (untested success path). - Bound the child-diagnostics sink. Cap the captured mockd stderr —
RLIMIT_FSIZEvia the child launcher, or a truncating/rotating sink — and record the size rationale next to the existing 0600 confidentiality rationale at runtime.py:41-45 (which reasons only about who can read the file, never about how large it can grow). Add a test that drives repeated handler tracebacks (peer disconnects mid-write =>BrokenPipeError=>ThreadingMixIn.handle_error=>print_exc) and asserts the sink stays bounded. A grep-level companion is possible (subprocess.Popen(..., stderr=<file object>)must be accompanied by a size cap, in the CE015 "unbounded stream from a child" family) but would be a single-site rule today. Why not static: Whether the sink is agent-influenceable depends on socket reachability (0660uip-rpc, andhandle()'s early-return paths do not consume themax_requestsbudget) and on which handler paths raise — a reachability property of the running system, not a syntactic one. Prevents: A6 low (unbounded, never-rotated, agent-drivable on-disk sink for the life of a run). - Record the mechanically-unreachable residue in the reviewer checklist (
.claude/shared/): three findings in this batch have no static or test lever and are deliberately left to human review — (1) the redundantnot argvdisjunct at server.py:129, which requires knowing_expand_argv_tokens([]) == []to see that the first operand can never independently trip; (2) thewaited/deadline_notecomputation at runtime.py:79-83, dead only because thewhile ... elsefall-through implieswaited >= STARTUP_TIMEOUT_SECONDSso both numbers always render identically; (3) the subset-ordering contract restated verbatim at server.py:114-116, server.py:215-217 and docs/TASK_DEFINITION_GUIDE.md:584. Keeping these on an explicit list marks the boundary of the mechanical gate as a decision rather than an omission. Why not static: Each needs semantic reasoning about a helper's behavior on a specific input, loop-exit implications, or prose equivalence — ruff/pyright/AST rules cannot express any of the three without effectively re-deriving the function's semantics. Prevents: A1 low x3 (redundant disjunct, duplicated timeout value, triplicated ordering comment).
Top 5 Priority Actions
- Stop passing subset rule argv through the invocation-side noise-flag scanner at src/coder_eval/protected_mock/server.py:128 — a bare
--outputin a rule eats the following rule token (["--output","rpa","get-errors"]loads as the one-token rule('get-errors',)), silently widening which invocations receive the canned answer and thus changing a task's score for identical agent output. - Fix the resolution order so declared passthrough prefixes stay authoritative: at src/coder_eval/protected_mock/server.py:214-227 subset scanning outranks
passthrough_argv_prefixes, so a one-token rule like{"argv":["docsai"],"match_mode":"subset"}disablesuip docsai ask …entirely — move the prefix check above the subset scan (or spell out the full exact → normalized → subset → passthrough → default chain at docs/TASK_DEFINITION_GUIDE.md:584) and add the missing regression test. - Detect post-startup mockd death at src/coder_eval/protected_mock/runtime.py:85-96 —
returncodeis consulted only during startup and the captured stderr is unlinked unread, so a mockd crash mid-run turns every call into client exit 125 and scores as an agent failure; log ERROR with_server_stderr_suffix(and set a non-success run status) before unlinking, and wrap the post-killprocess.wait(timeout=5)at line 92 so cleanup can never be skipped. - Parse fixture response entries with a Pydantic model (
extra="forbid",match_mode: Literal["exact","normalized","subset"],exit_code: int = Field(0, ge=0, le=255)) at src/coder_eval/protected_mock/server.py:124 — today a typo'dmatch_modeskey loads clean and silently degrades a subset rule to exact matching (so every real invocation falls through todefault), and a non-hashablematch_moderaisesTypeErrorbefore the descriptiveValueErroron line 126. - Close the test gaps that let all of the above ship green: add flag-bearing subset cases at tests/test_protected_mock.py:213 (a
--job-id 42rule must not answer a--job-id 99invocation, plus the--job-id=inline form covering the uncovered58->50branch), a success-path andPopen-raises-OSErrortest forrunning_mock_server(src/coder_eval/protected_mock/runtime.py:61-63, module at 76.47% — deleting the leak-guard unlink today leaves the suite passing), and a one-line load test for the three-mode rejection message at server.py:126.
Stats: 0 🔴 · 3 🟠 · 6 🟡 · 10 🔵 across 8 axes reviewed.
2 participants
Stacked on #87. Ports the load-bearing pieces of #90 onto #87's protected_mock so #87 becomes the single vehicle and #90 can be closed as superseded.
What is ported
match_mode: subset(server.py): token-subset matching evaluated below exact/normalized, scanned in fixture-file order, first match wins, duplicates legal (an earlier rule shadows a later one), empty or noise-only rule argv rejected at load. Real-agent measurement showed finite matching rejects 37% of actualuipinvocations over benign extra flags; the skills troubleshoot corpus (Migrate troubleshoot fixtures to protected mocks skills#2503, 297 scenarios) is regenerated against this schema, replay-validated on 379 recorded argvs.--outputnoise flag no longer swallows a following flag (deploy --output --delete-allno longer normalizes to baredeploy), and--output=<empty>is dropped atomically instead of consuming the next unrelated token.What is deliberately NOT ported
Validation
uip rpa get-errors --output json --limit 5 --folder-key demo): the subset rule answered through the full mockd RPC path and the task passed 1/1 (exact/normalized cannot match that argv, so the pass is attributable to the ported matcher). The same task against the pre-port image fails with mockd exiting at fixture load - the old matcher rejectsmatch_mode: subset, which also demonstrates the diagnosability gap the startup hardening addresses.