Repository navigation
[codex] Update approval E2E expectations - #3054
Conversation
github-actions
Bot
added
size: XS
There was a problem hiding this comment.
Pull request overview
Updates E2E expectations around tool approval gates after the default HTTP tool permission changed, ensuring tests that depend on a real approval pause explicitly configure the HTTP tool to require per-call approval.
Changes:
- Update the builtin tool coverage assertion to match the current
tool_searchguidance:tool_activate(name="..."). - Add a small E2E helper to force the HTTP tool permission to
ask_each_time, and call it before scenarios that need a real approval gate.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/e2e_builtin_tool_coverage.rs |
Aligns tool_search description assertion with the current tool_activate(name="...") guidance. |
tests/e2e/scenarios/test_tool_approval.py |
Ensures approval-gate-dependent E2Es explicitly configure HTTP tool permission to ask_each_time before triggering real HTTP tool calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -626,6 +651,7 @@ async def test_normal_text_not_intercepted_with_approval_card(page): | |||
|
|
|||
| async def test_text_approval_resolves_real_tool_call(browser, managed_gateway_server): | |||
There was a problem hiding this comment.
There are two test functions with the exact same name test_text_approval_resolves_real_tool_call in this module (one earlier taking page, and this one taking browser, managed_gateway_server). In Python the later definition overwrites the earlier one at import time, so one of these tests will never run under pytest. Rename one of the tests (and optionally adjust the docstring) so both scenarios are collected and executed.
| async def test_text_approval_resolves_real_tool_call(browser, managed_gateway_server): | |
| async def test_text_approval_resolves_real_tool_call_with_managed_gateway( | |
| browser, managed_gateway_server | |
| ): |
| @@ -838,6 +864,7 @@ async def handle_gate_resolve(route): | |||
| async def test_slash_approve_is_thread_scoped_api(managed_gateway_server): | |||
There was a problem hiding this comment.
There are two test functions named test_slash_approve_is_thread_scoped_api in this module (one using ironclaw_server, and this one using managed_gateway_server). The second definition overwrites the first, so pytest will only execute one of them. Rename one of the tests so both variants are collected.
| async def test_slash_approve_is_thread_scoped_api(managed_gateway_server): | |
| async def test_slash_approve_is_thread_scoped_api_managed_gateway(managed_gateway_server): |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates end-to-end tests by introducing a helper function _require_http_approval to standardize the configuration of HTTP tool approval gates across multiple test scenarios. It also updates a documentation string in the Rust test suite. However, the review identified critical issues where several test function names are duplicated within tests/e2e/scenarios/test_tool_approval.py, which will cause pytest to silently overwrite and skip the initial test definitions. These function names must be made unique to ensure all test cases are executed.
|
|
||
| async def test_text_approval_resolves_real_tool_call(page): | ||
| """Typing 'yes' should resolve a real approval gate triggered by a tool call.""" | ||
| await _require_http_approval(_page_base_url(page)) |
There was a problem hiding this comment.
The test function name test_text_approval_resolves_real_tool_call is duplicated in this file (see line 316 and line 652). In Python, the second definition will overwrite the first, meaning the test case at line 316 will be silently skipped by pytest. Please rename these functions to be unique (e.g., by appending _simple and _gateway).
|
|
||
| async def test_slash_approve_is_thread_scoped_api(ironclaw_server): | ||
| """Sending '/approve' in thread A must not resolve a pending gate in thread B.""" | ||
| await _require_http_approval(ironclaw_server) |
There was a problem hiding this comment.
The test function name test_slash_approve_is_thread_scoped_api is duplicated in this file (see line 346 and line 864). The second definition will overwrite the first, causing the first test implementation to be skipped. Please rename them to be unique.
Summary
Root Cause
Commit 8e54e51 moved the HTTP tool default to AlwaysAllow, so fresh E2E servers no longer produced pending approval gates for tests that implicitly depended on the old default.
Validation