fix(codex): preserve web telemetry and isolate task skills#47
fix(codex): preserve web telemetry and isolate task skills#47apetraru-uipath wants to merge 2 commits into
Conversation
|
Claude finished @apetraru-uipath's task in 1m 25s —— View job Code Review in Progress
|
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:47
Scope: pr:47 · branch codex/codex-webfetch-telemetry · 132703d · 2026-07-23T08:17Z · workflow variant
Change class: complex — reclassifies Codex webSearch page-opens (openpage/findinpage/open_page/find_in_page) as WebFetch vs WebSearch and upgrades the provisional ToolStart CommandTelemetry in place at item/completed; correctness reasoning required
coder_eval is in excellent health — 10/10 across quality, type safety, security, architecture, error handling, and harness correctness, with the only blemishes two low-severity maintainability nits (a missing snake_case test case and a dead lookup entry) that pose no scoring or final_status risk, so the bottom line is ship-as-is with a couple of near-zero-cost cleanups.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 2. Type Safety | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 3. Test Health | 9.9 / 10 | 0 | 0 | 0 | 1 | New tool-parsing formats (WebFetch action types, webSearch result_summary) lack test assertions |
| 4. Security | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 5. Architecture & Design | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 6. Error Handling & Resilience | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 7. API Surface & Maintainability | 9.9 / 10 | 0 | 0 | 0 | 1 | Dead _TOOL_ITEM_NAMES["webSearch"] entry after webSearch is special-cased in _tool_name |
| 8. Evaluation Harness Quality | 10 / 10 | 0 | 0 | 0 | 0 | — |
Overall Score: 10 / 10 · Weakest Axis: Test Health at 9.9 / 10
Totals: 🔴 0 · 🟠 0 · 🟡 0 · 🔵 2 across 8 axes.
Blockers
None.
Non-blocking, but please consider before merge
None.
Nits
- [Axis 3] New tool-parsing formats (WebFetch action types, webSearch result_summary) lack test assertions (
tests/test_codex_agent.py:1353) —_WEB_FETCH_ACTION_TYPES = frozenset({"openpage", "findinpage", "open_page", "find_in_page"})(codex_agent.py:174) has four members, but the tests only exercise the camelCase forms. The parametrize at tests/test_codex_agent.py:1357-1361 lists onlytype="openPage"andtype="findInPage"(which normalize to openpage/findinpage), and the unit tests (tests/test_codex_agent_unit.py:117,135) also use onlyopenPage. The snake_case membersopen_page/find_in_page— deliberately included in the frozenset to defend against SDK form uncertainty — go through the identical_status_value(...) in _WEB_FETCH_ACTION_TYPESmembership check but are never asserted, so removing or typo'ing either snake_case entry would silently drop a classification with no failing test. Addtype="open_page"andtype="find_in_page"cases to the existing parametrize (near-zero cost, closes the classification-set parity gap). - [Axis 7] Dead
_TOOL_ITEM_NAMES["webSearch"]entry after webSearch is special-cased in_tool_name(src/coder_eval/agents/codex_agent.py:138) —_tool_namenow intercepts webSearch before the generic map: lines 1549-1550if root_type == "webSearch": return _web_search_tool_name(root)run before line 1551return _TOOL_ITEM_NAMES.get(...). Since_tool_name(line 1551) is the ONLY consumer of_TOOL_ITEM_NAMES(confirmed by grep — only definition at line 132 and the one.getat 1551), the entry"webSearch": "WebSearch",(line 138) is now unreachable. It is harmless (its value matches the non-page-action default of_web_search_tool_name) but misleads a reader into thinking the webSearch label comes from the generic map when it is actually derived from the item's action type. Remove line 138 and add a one-line comment on_TOOL_ITEM_NAMESnoting webSearch is special-cased in_tool_name, so the two labelling paths don't silently drift.
What's Missing
Downstream consumers:
- 🟡 The Codex tool-name label is a consumed classification, not just a display string: command_executed matches it exactly (criteria/command_executed.py:86,
cmd.tool_name == criterion.tool_name) and analysis.py buckets on it (commands_by_tool, analysis.py:32/66). Relabeling Codex page-opens fromWebSearchtoWebFetchmeans any command_executed/skill_triggered criterion keyed ontool_name: WebSearchfor a page-open now flips pass/fail for identical Codex output. No in-repo task uses these labels today (grep of tasks/ is clean), so it is latent rather than live-breaking, but the relabel is exactly the Axis-8 'same output, different score' class. The PR should state the classification change and audit tool-name-keyed consumers. (trigger: src/coder_eval/agents/codex_agent.py)
Daily/nightly:
- 🟡 CommandTelemetry.tool_name is part of the run-record/task.json contract consumed by the separate coder-eval-uipath / eval-runner nightly pipeline and its dashboards (per CLAUDE.md, that pipeline now lives in another repo). Introducing a new
WebFetchvalue for Codex changes commands_by_tool aggregation and any tool-name-keyed criteria in the out-of-repo nightly suites, under version skew. The PR does not state this cross-repo blast radius; an unstated nightly impact is itself the gap. (trigger: src/coder_eval/agents/codex_agent.py)
Parallel paths:
- 🔵 antigravity_agent.py:114 maps
search_web -> WebSearchand its comment (lines 100-104) explicitly states antigravity keys on 'the SAME tool names the Claude / Codex backends emit' so cross-agent criteria stay consistent. Codex now emittingWebFetchfor page-open actions diverges that parallel agent tool-labeling path — antigravity has no equivalent page-open sub-action, so this may be inherent rather than a missed update, but the divergence against the documented cross-agent-consistency intent should be confirmed, not assumed. (trigger: src/coder_eval/agents/codex_agent.py)
Tests:
- 🔵 Missing assertions for the newly-added tool-parsing behavior: the snake_case action members (open_page / find_in_page) of _WEB_FETCH_ACTION_TYPES and the webSearch result_summary path are never asserted, so removing/typo'ing either silently drops a classification with no failing test. Add the snake_case cases to the existing parametrize and a result_summary assertion. (trigger: tests/test_codex_agent.py) (restates: Axis 3: New tool-parsing formats (WebFetch action types, webSearch result_summary) lack test assertions)
Display & mapping dicts:
- 🔵 No reports*.py per-tool icon/label dict needs a new
WebFetchentry — reports render tool names generically (grep of reports.py/reports_experiment.py for tool-icon maps is empty), so there is no fall-through-to-'?' gap. The only mapping-dict concern is the now-unreachable _TOOL_ITEM_NAMES['webSearch'] entry left behind after webSearch is special-cased in _tool_name. (trigger: src/coder_eval/agents/codex_agent.py) _(restates: Axis 7: Dead _TOOL_ITEM_NAMES[webSearch] entry after webSearch is special-cased in tool_name)
Harness & Lint Improvements
Static checks (lint / type):
- (none)
Harness improvements (not statically reachable):
- Make the WebFetch action-type test in tests/test_codex_agent.py self-covering by parametrizing directly over the source frozenset instead of a hand-maintained literal list:
@pytest.mark.parametrize("action_type", sorted(_WEB_FETCH_ACTION_TYPES))(importing_WEB_FETCH_ACTION_TYPESfrom codex_agent), asserting each form classifies as a WebFetch. Any member added to (or typo'd out of) the frozenset then automatically gains/loses a test case, closing the classification-set parity gap permanently rather than for just the two camelCase forms. Why not static: A lint/type rule cannot verify that each frozenset member is exercised by a test: it would have to correlate the source set literal with test parametrize values and actually run the parser to observe the resulting classification, which is runtime/semantic, not grep- or AST-detectable. Prevents: Finding 1 (tests/test_codex_agent.py:1353) — snake_case members open_page/find_in_page in _WEB_FETCH_ACTION_TYPES go through the identical membership check but are never asserted, so removing/typo'ing either would silently drop a classification with no failing test; also guards any future member added to the set.
Top 5 Priority Actions
- Add
type="open_page"andtype="find_in_page"cases to the WebFetch action-type parametrize at tests/test_codex_agent.py:1357 so the snake_case members of_WEB_FETCH_ACTION_TYPES(src/coder_eval/agents/codex_agent.py:174) are actually asserted and a typo can't silently drop a classification. - Remove the now-unreachable
"webSearch": "WebSearch"entry in_TOOL_ITEM_NAMESat src/coder_eval/agents/codex_agent.py:138, since_tool_namespecial-cases webSearch before consulting the map, and add a one-line comment noting the special-casing to prevent the two labelling paths from drifting. - Also cover the webSearch
result_summaryparsing path flagged near tests/test_codex_agent.py:1324 with an explicit assertion, closing the parity gap for newly-added tool-parsing behavior. - Consider a small CE lint rule asserting every member of a classification frozenset like
_WEB_FETCH_ACTION_TYPESis exercised by at least one parametrized test, turning this one-off coverage fix into permanent enforcement per the CLAUDE.md lint-rule guidance. - No further action required on the remaining seven axes (Code Quality, Type Safety, Security, Architecture, Error Handling, API Surface, Evaluation Harness), which are clean at 10/10 — hold the line rather than add speculative changes.
Stats: 0 🔴 · 0 🟠 · 0 🟡 · 2 🔵 across 8 axes reviewed.

Summary
Validation
The changes keep WebFetch criteria strict across Claude and Codex and make guardrail skill provenance deterministic without weakening task definitions.