fix: integration test mandatory for instrumentation and cross-cutting PRs #232

Problem

PR #232 had 17 unit tests that all passed, but the actual instrumentation was completely non-functional because:

• The monkey-patch did not reach imported consumers
• ru_maxrss was used as instantaneous RSS
• Token counts were structurally unobtainable

All 17 tests tested the helper classes (MetricsCollector, RSSSampler, percentile) in isolation. No test exercised the full path: smoke script → engine → LLM client → metrics collector.

Root Cause

1. "Tests pass" was treated as "implementation correct": The agent ran make check, saw green, and concluded the work was done. But the tests only verified the helpers, not the integration.
2. No requirement for integration tests in the task brief: The handoff document asked for make health and make check but did not explicitly require an integration test for the instrumented code path.
3. Agent did not ask "how do I prove this actually works end-to-end?"

Prevention

For the agent (workflow rule)

1. For any PR that instruments, wraps, or intercepts existing code paths, include at least one integration test that exercises the full chain. The test must fail if the instrumentation is removed or broken.
2. Integration test minimum requirements for instrumentation PRs:
   • A mock that exercises the instrumented code path and proves the collector receives data
   • A test that the instrumentation restores state after normal completion AND after exceptions
   • A test that would fail on the specific bug being fixed (e.g., if the patch misses imported names, the test must detect zero-capture)
3. Apply the "test what you claim" rule: If the PR claims to capture X, the test must demonstrate X being captured through the real code path (mocked I/O is fine, but real call chain is required).

For the handoff/review template

4. Add a checklist item: "Integration test proves the instrumented code path captures real calls?"
5. Add a checklist item: "Fail-fast check: does the script error if zero calls are captured in a non-trivial run?"

Example test that would have caught PR #232 bugs

@pytest.mark.asyncio
async def test_smoke_metrics_captures_real_llm_calls():
    """Prove the instrumented path reaches the metrics collector."""
    collector = MetricsCollector()

    # Use the real code path: conversation_agent → get_llm_client → generate
    with instrumented_llm(collector):
        # Mock the HTTP call, but use real conversation_agent.run_conversation
        mock_client = AsyncMock()
        mock_client.generate = AsyncMock(return_value="test response")
        result = await run_conversation(
            persona_a=..., persona_b=...,
            ..., client=mock_client,
        )

    # This assertion would FAIL on the original PR #232
    assert collector.turn_count > 0, "Instrumentation did not capture LLM calls"
    assert collector.latencies() != [], "No latency data captured"

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: fix: AI agent instrumentation must use source-level hooks, not monkey-patching of already-imported names #228 (monkey-patch vs hooks)