From 5d25abd356a8872d5ac3c941c61ebbd853c35b4d Mon Sep 17 00:00:00 2001 From: Tejas Chopra Date: Fri, 21 Aug 2026 23:18:40 -0700 Subject: [PATCH] test: repair three suite failures that are red on main (#3196) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Three tests fail on a clean `main` full-suite run. None is a product defect — all three are tests that stopped describing reality, and they will noise up or block the 0.36.4 release. | Test | Why it fails | Fix | |---|---|---| | `test_release_workflows::test_no_native_tls_in_wheel_build_tree` | Shells out to `cargo`; raises `FileNotFoundError` wherever the Rust toolchain is absent | Copied the skip guards its own dual already had | | `test_learn/test_integration::TestCodexIntegration::test_full_pipeline` | Asserts `"Bash" in all_tools` against **real local Codex data**; Codex renamed its shell tool | Assert what the test is for, across Codex versions | | `test_graceful_shutdown::test_run_server_installs_cancelled_error_filter` | Counts installs on the **process-global** `uvicorn.error` logger; order-dependent | Isolate the global state; assert the real contract | ## 1. native-tls / cargo The `openssl-sys` gate 30 lines above is described in-code as this test's dual. It already skips when `cargo` is missing, **and** when cargo fails for a reason other than `"package did not match"` (the Linux wheel target not being installed locally). The native-tls test never copied either guard. Not disabled: CI installs the toolchain via `dtolnay/rust-toolchain`, so the check still executes there. The skip only applies where cargo is genuinely absent. ## 2. Codex tool vocabulary This test runs against whatever Codex sessions the machine actually has (gated by `HAS_CODEX_DATA`), and asserted: ```python # Codex has only Bash tool (shell) assert "Bash" in all_tools ``` Codex has since renamed its shell tool (`Bash` → `shell` → `exec`), and 0.149.0 added agent tools (`spawn_agent`, `send_message`, `wait`) beside it. The assertion pinned one release's vocabulary, so it fails on any current install. It now asserts what the pipeline is actually being tested for — that tool calls were extracted, including a shell-execution tool under any of its known names — and names the remedy in the failure message for the next rename. **Still discriminating** (verified, not assumed): | Scenario | Result | |---|---| | pipeline parsed nothing | fails ✓ | | tool names garbled | fails ✓ | | agent tools only, no shell tool | fails ✓ | | real current Codex data | passes ✓ | ## 3. Global logger state ```python if not any(isinstance(item, _SuppressCancelledErrorFilter) for item in uvicorn_error_logger.filters): uvicorn_error_logger.addFilter(_SuppressCancelledErrorFilter()) ``` `run_server` is deliberately idempotent and `uvicorn.error` is a process-global logger, so any earlier test in the session that reached `run_server` leaves the filter attached — and this test then observes **zero** installs against its `== 1` assertion. It passes alone and fails in a full run, which is exactly the symptom. The test now clears and restores that global state around itself, and additionally asserts the idempotence guard that is the real contract: calling `run_server` twice must not stack a duplicate filter. The test got stronger, not just quieter. ## Scope Tests only — no product code is touched. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Tejas Chopra Co-authored-by: Claude Opus 5 --- tests/test_graceful_shutdown.py | 35 ++++++++++++++++++++-- tests/test_learn/test_integration.py | 15 ++++++++-- tests/test_release_workflows.py | 44 ++++++++++++++++++---------- 3 files changed, 74 insertions(+), 20 deletions(-) diff --git a/tests/test_graceful_shutdown.py b/tests/test_graceful_shutdown.py index 7bff1e5a1..24358295d 100644 --- a/tests/test_graceful_shutdown.py +++ b/tests/test_graceful_shutdown.py @@ -109,9 +109,40 @@ def test_run_server_installs_cancelled_error_filter(monkeypatch: pytest.MonkeyPa from headroom.proxy.server import run_server - run_server(ProxyConfig(), print_banner=False) + # `uvicorn.error` is a process-global logger and run_server only installs + # the filter when one is not already attached. Any earlier test in the + # session that reached run_server therefore leaves it installed, and this + # test would observe zero installs. Isolate the global state rather than + # depend on test ordering. + uvicorn_error_logger = logging.getLogger("uvicorn.error") + preexisting = [ + item + for item in uvicorn_error_logger.filters + if isinstance(item, _SuppressCancelledErrorFilter) + ] + for item in preexisting: + uvicorn_error_logger.removeFilter(item) - assert len(installed_filters) == 1, "Expected exactly one _SuppressCancelledErrorFilter" + try: + run_server(ProxyConfig(), print_banner=False) + + assert len(installed_filters) == 1, "Expected exactly one _SuppressCancelledErrorFilter" + + # The idempotence guard is the real contract: a second call must not + # stack a duplicate filter on the shared logger. + run_server(ProxyConfig(), print_banner=False) + attached = [ + item + for item in uvicorn_error_logger.filters + if isinstance(item, _SuppressCancelledErrorFilter) + ] + assert len(attached) == 1, f"filter stacked on repeat calls: {len(attached)}" + finally: + for item in list(uvicorn_error_logger.filters): + if isinstance(item, _SuppressCancelledErrorFilter): + uvicorn_error_logger.removeFilter(item) + for item in preexisting: + original_add_filter(uvicorn_error_logger, item) # --------------------------------------------------------------------------- diff --git a/tests/test_learn/test_integration.py b/tests/test_learn/test_integration.py index 3ed01d189..be465a612 100644 --- a/tests/test_learn/test_integration.py +++ b/tests/test_learn/test_integration.py @@ -294,9 +294,20 @@ class TestCodexIntegration: assert len(sessions) > 0 - # Codex has only Bash tool (shell) + # This runs against whatever Codex sessions the machine actually has, + # so it must not pin one release's tool vocabulary. Codex has renamed + # its shell tool across versions (`Bash` -> `shell` -> `exec`) and + # 0.149.0 added agent tools alongside it, which is what the pipeline + # has to keep parsing. all_tools = {tc.name for s in sessions for tc in s.tool_calls} - assert "Bash" in all_tools + assert all_tools, "pipeline extracted no tool calls from real Codex sessions" + + shell_tool_aliases = {"Bash", "shell", "exec", "local_shell", "container.exec"} + assert all_tools & shell_tool_aliases, ( + "no shell-execution tool recognised in real Codex sessions; " + f"saw {sorted(all_tools)} -- if Codex renamed it again, add the " + "new name to shell_tool_aliases" + ) def test_codex_writer_targets_agents_md(self, tmp_path): """Codex writer should target AGENTS.md, not CLAUDE.md.""" diff --git a/tests/test_release_workflows.py b/tests/test_release_workflows.py index b3b3a9c07..732edda23 100644 --- a/tests/test_release_workflows.py +++ b/tests/test_release_workflows.py @@ -339,23 +339,35 @@ def test_no_native_tls_in_wheel_build_tree() -> None: import subprocess for crate in ("headroom-py", "headroom-proxy", "headroom-core"): - result = subprocess.run( - [ - "cargo", - "tree", - "--target", - "x86_64-unknown-linux-gnu", - "-p", - crate, - "-i", - "native-tls", - ], - cwd=str(ROOT), - capture_output=True, - text=True, - check=False, - ) + try: + result = subprocess.run( + [ + "cargo", + "tree", + "--target", + "x86_64-unknown-linux-gnu", + "-p", + crate, + "-i", + "native-tls", + ], + cwd=str(ROOT), + capture_output=True, + text=True, + check=False, + ) + except FileNotFoundError: + pytest.skip("cargo is unavailable in this environment") + # Same environment gates as the openssl-sys dual above: a cargo + # failure that is not "package did not match" means the Linux wheel + # target is unavailable here, not that native-tls came back. not_in_tree = result.returncode != 0 and "did not match any packages" in result.stderr + if result.returncode != 0 and "package ID specification `native-tls` did not match" not in ( + result.stderr + result.stdout + ): + pytest.skip( + "cargo dependency tree for the Linux wheel target is unavailable in this environment" + ) assert not_in_tree, ( f"native-tls is back in {crate}'s build tree — likely some " f"crate's `default-features = true` re-enabled native-tls "