fix: AI agent instrumentation must use source-level hooks, not monkey-patching of already-imported names #228

Problem

When the OpenCode agent (implement mode) was tasked with instrumenting LLM calls in persona-village-v2 PR #232, it chose to monkey-patch utils.llm_client.get_llm_client at the module level. However, Python from X import Y copies the reference at import time, so consumer modules like agents.conversation.conversation_agent already held a reference to the original function. The patch had zero effect on actual LLM calls.

The unit tests passed because they only tested the metrics helper classes in isolation — no integration test verified that the instrumented code path was actually reached.

Root Cause

1. Insufficient understanding of Python import semantics: from module import func binds func in the local namespace. Rebinding module.func later does not update the local binding.
2. No integration test requirement: The agent considered unit tests sufficient and did not create a test that exercises the full instrumented code path.
3. Over-constraint interpretation: The instruction "do not modify engine code" was interpreted as "zero changes to engine files" rather than "minimal changes," leading to a fragile wrapper instead of a small, safe hook point.

Prevention

For the agent (opencode guardrail)

1. When instrumenting existing code, prefer source-level hooks over monkey-patching. If a hook point does not exist, add a minimal one (e.g., an optional callback, a context variable, or an event) rather than wrapping from the outside.
2. Always include an integration regression test that proves the instrumentation actually captures calls through the real code path. A unit test of the helper alone is insufficient.
3. Verify runtime behavior, not just code structure. If the instrumentation claims to capture X, the test must demonstrate X being captured in a scenario that exercises the actual call chain.

For the review checklist

4. Monkey-patch warning: If a PR uses monkey-patching as the primary instrumentation mechanism, flag it for extra scrutiny. Ask: "Does this patch reach all consumers, including those that imported by name?"
5. Integration test gate: Any PR that adds metrics/telemetry instrumentation must include at least one test that exercises the full instrumented path, not just helper unit tests.

Example

Bad (PR #232 original):

# Patches only the module-level reference, misses already-imported copies
import utils.llm_client as _mod
_mod.get_llm_client = _instrumented

Good (PR #232 rework):

# Add a process-local hook in the source
# utils/llm_client.py
_ACTIVE_HOOK = None

def set_generate_hook(hook):
    global _ACTIVE_HOOK
    _ACTIVE_HOOK = hook

# Inside generate():
if _ACTIVE_HOOK:
    _ACTIVE_HOOK(model, latency_s, usage)

References

• PR fix: integration test mandatory for instrumentation and cross-cutting PRs #232 review: https://github.com/Cor-Incorporated/persona-village-v2/pull/232#issuecomment-4525827375
• Related upstream epic: epic: internal AI guardrails thin distribution for Cor-Incorporated #1