fix: agent must use proper scope management for instrumentation, not permanent global patches #231

Problem

PR #232 used a bare function _patch_llm_client() that permanently replaced utils.llm_client.get_llm_client for the entire process lifetime. If the smoke run raised an exception, the patch remained installed. If called twice, it would double-wrap clients. There was no context manager, no try/finally restoration, and no cleanup.

Additionally, InstrumentedLLMClient inherited from OpenAICompatibleClient without calling super().__init__, leaving all inherited attributes uninitialized. It also accessed the private _wrapped._default_model attribute directly.

Root Cause

1. "Script-only" mentality: The agent treated the smoke script as a one-shot throwaway runner and did not consider lifecycle management.
2. Inheritance without contract: The agent used inheritance to satisfy type-checking but did not fulfill the base class contract. This is a code smell that the agent did not recognize.
3. Private attribute coupling: Accessing _default_model from another class creates hidden coupling that breaks silently if the attribute is renamed.

Prevention

For the agent

1. Any instrumentation scope must be a context manager with try/finally restoration. This applies to monkey-patches, mock installations, signal handlers, and any other global state mutation.
2. Do not inherit from a class without fulfilling its contract. If only a subset of methods is needed, use composition or duck-typing (Protocol) instead.
3. Do not access private attributes (_name) from outside the owning class. If the public API does not expose the needed data, either:
   • Add a public property/method to expose it (minimal change), or
   • Pass the data explicitly at construction time.

Design pattern for scoped instrumentation

from contextlib import contextmanager

@contextmanager
def instrumented_llm(collector):
    old = utils.llm_client._ACTIVE_HOOK
    try:
        utils.llm_client._ACTIVE_HOOK = collector.record_turn
        yield collector
    finally:
        utils.llm_client._ACTIVE_HOOK = old

References

• PR fix: integration test mandatory for instrumentation and cross-cutting PRs #232 review H1, H4: https://github.com/Cor-Incorporated/persona-village-v2/pull/232#issuecomment-4525827375