Skip to content

fix(codex): preserve web telemetry and isolate task skills#47

Open
apetraru-uipath wants to merge 2 commits into
mainfrom
codex/codex-webfetch-telemetry
Open

fix(codex): preserve web telemetry and isolate task skills#47
apetraru-uipath wants to merge 2 commits into
mainfrom
codex/codex-webfetch-telemetry

Conversation

@apetraru-uipath

@apetraru-uipath apetraru-uipath commented Jul 23, 2026

Copy link
Copy Markdown

Summary

  • classify Codex webSearch openPage/findInPage actions as canonical WebFetch
  • preserve page URL/pattern telemetry while retaining searches as WebSearch
  • run Codex with a private per-agent HOME/CODEX_HOME so task-local skills cannot be shadowed by stale user-global skills
  • copy auth.json into the private runtime with restrictive permissions; never link mutable credentials
  • isolate sub-agent rollouts and clean runtime state across stop, kill, retry, and startup failure paths
  • preserve host HOME/USERPROFILE only for tool subprocesses, including Windows

Validation

  • focused Codex agent tests: 126 passed
  • custom architectural lint: 116 passed
  • Ruff format/check: passed
  • Pyright: 0 errors (one pre-existing Antigravity warning)
  • independent security/portability review: approved
  • git diff --check and pre-commit hooks: passed

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

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

Claude finished @apetraru-uipath's task in 1m 25s —— View job


Code Review in Progress

Todo List:

  • Read .github/code_review.md for review standards
  • Read CLAUDE.md for project conventions
  • Review full PR diff (origin/main...HEAD)
  • Read full context of changed files
  • Analyze changes for correctness and design
  • Check cross-file consistency
  • Perform "what's missing" analysis
  • Post structured review feedback

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: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

  1. [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 only type="openPage" and type="findInPage" (which normalize to openpage/findinpage), and the unit tests (tests/test_codex_agent_unit.py:117,135) also use only openPage. The snake_case members open_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_TYPES membership check but are never asserted, so removing or typo'ing either snake_case entry would silently drop a classification with no failing test. Add type="open_page" and type="find_in_page" cases to the existing parametrize (near-zero cost, closes the classification-set parity gap).
  2. [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_name now intercepts webSearch before the generic map: lines 1549-1550 if root_type == "webSearch": return _web_search_tool_name(root) run before line 1551 return _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 .get at 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_NAMES noting 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 from WebSearch to WebFetch means any command_executed/skill_triggered criterion keyed on tool_name: WebSearch for 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 WebFetch value 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 -> WebSearch and 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 emitting WebFetch for 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 WebFetch entry — 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_TYPES from 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

  1. Add type="open_page" and type="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.
  2. Remove the now-unreachable "webSearch": "WebSearch" entry in _TOOL_ITEM_NAMES at src/coder_eval/agents/codex_agent.py:138, since _tool_name special-cases webSearch before consulting the map, and add a one-line comment noting the special-casing to prevent the two labelling paths from drifting.
  3. Also cover the webSearch result_summary parsing path flagged near tests/test_codex_agent.py:1324 with an explicit assertion, closing the parity gap for newly-added tool-parsing behavior.
  4. Consider a small CE lint rule asserting every member of a classification frozenset like _WEB_FETCH_ACTION_TYPES is exercised by at least one parametrized test, turning this one-off coverage fix into permanent enforcement per the CLAUDE.md lint-rule guidance.
  5. 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.

@apetraru-uipath apetraru-uipath changed the title fix(codex): classify page opens as WebFetch fix(codex): preserve web telemetry and isolate task skills Jul 23, 2026
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