Skip to content

fix: _is_openai_client_closed false positive with openai SDK method-style is_closed#4378

Open
saulmc wants to merge 1 commit intoNousResearch:mainfrom
saulmc:fix/is-openai-client-closed-false-positive
Open

fix: _is_openai_client_closed false positive with openai SDK method-style is_closed#4378
saulmc wants to merge 1 commit intoNousResearch:mainfrom
saulmc:fix/is-openai-client-closed-false-positive

Conversation

@saulmc
Copy link
Copy Markdown

@saulmc saulmc commented Apr 1, 2026 •
edited
Loading

Summary

  • Fix _is_openai_client_closed to call is_closed() when it's callable instead of checking truthiness of the bound method object
  • Add regression test with a method-style is_closed client

Why

The openai SDK (>=2.21.0) exposes is_closed as a method, not a property:

# openai/_base_client.py
class SyncAPIClient:
    def is_closed(self) -> bool:
        return self._client.is_closed

The current check bool(getattr(client, "is_closed", False)) returns True for the bound method object itself (all callables are truthy), so every API call falsely detects the shared client as closed and recreates it — adding an unnecessary TCP+TLS round trip per call to replace a healthy client.

Existing tests don't catch this because FakeRequestClient doesn't define is_closed, so getattr returns the False default and falls through to the _client.is_closed check which works correctly.

Fixes #4377
@saulmc saulmc force-pushed the fix/is-openai-client-closed-false-positive branch from a76c791 to 9262fef Compare April 1, 2026 00:06
@saulmc
fix: _is_openai_client_closed false positive with openai SDK method-s…
340cb04 
…tyle is_closed

The openai SDK (>=2.21.0) exposes is_closed as a method, not a
property. bool(getattr(client, "is_closed", False)) returns True for
the bound method object itself, causing every API call to falsely
detect the shared client as closed and recreate it — adding ~2 extra
TCP+TLS round trips per call.

Call is_closed() when callable; check the return value otherwise.
Handles both method-style (openai SDK) and attribute-style
(CopilotACPClient, test fakes).

Fixes NousResearch#4377
@saulmc saulmc force-pushed the fix/is-openai-client-closed-false-positive branch from 9262fef to 340cb04 Compare April 1, 2026 00:34
saulmc added a commit to xmtplabs/convos-assistants that referenced this pull request Apr 1, 2026
@saulmc @claude
fix(hermes): handle agent: null in config.yaml + align monkey-patch w…
4baec69 
…ith upstream PR

- Use (cfg.get("agent") or {}) to handle null values so the
  HERMES_REASONING_EFFORT env var fallback still works
- Align monkey-patch with the version submitted upstream
  (NousResearch/hermes-agent#4378)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
saulmc added a commit to xmtplabs/convos-assistants that referenced this pull request Apr 1, 2026
@saulmc @claude
fix(hermes): handle agent: null in config.yaml + align monkey-patch w…
a4e7361 
…ith upstream PR

- Use (cfg.get("agent") or {}) to handle null values so the
  HERMES_REASONING_EFFORT env var fallback still works
- Align monkey-patch with the version submitted upstream
  (NousResearch/hermes-agent#4378)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
saulmc added a commit to xmtplabs/convos-assistants that referenced this pull request Apr 1, 2026
@saulmc @claude
fix(hermes): wire config.yaml through to AIAgent + fix client is_clos…
1fc2e57 
…ed false positive (#809)

* fix(hermes): wire config.yaml through to AIAgent + fix client is_closed false positive

AgentRunner bypassed several config.yaml sections that the upstream
Hermes gateway/cli properly pass to AIAgent:

- provider_routing (only, ignore, order, sort, require_parameters, data_collection)
- fallback_providers / fallback_model
- agent.reasoning_effort

This meant our provider_routing.only restriction was dead config —
OpenRouter could route to any provider.

Also monkey-patches _is_openai_client_closed to fix an upstream bug
where openai SDK's is_closed (a method) is checked via
bool(getattr(client, "is_closed", False)) — the bound method is always
truthy, so every API call falsely detects the shared client as closed
and recreates it, adding ~2 unnecessary TCP+TLS round trips per call.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(hermes): handle agent: null in config.yaml + align monkey-patch with upstream PR

- Use (cfg.get("agent") or {}) to handle null values so the
  HERMES_REASONING_EFFORT env var fallback still works
- Align monkey-patch with the version submitted upstream
  (NousResearch/hermes-agent#4378)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* chore(hermes): replace [all] with specific extras for hermes-agent

.[all] installs every optional dependency (Matrix, Telegram, Discord,
Slack, SMS, DingTalk, voice, Modal, etc.) — most of which we don't
use. The matrix extra in particular requires python-olm which needs
cmake and C++ compilation, breaking local dev on macOS.

Install only the extras we actually use: cron, honcho, mcp, pty.
Applied to both Dockerfile and install-deps.sh.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* chore(hermes): remove unused honcho session key from AIAgent init

We don't use honcho and just removed the dependency.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(hermes): remove @staticmethod from monkey-patch function

staticmethod descriptor assigned to an instance attribute isn't
callable — would raise TypeError at runtime.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant

@saulmc