fix(memory): batch-wrap create_observation and remove observation auto-creation

[ryanrishi]

Summary

Related fixes to the Conversation Memory observation flow (Python parity for twilio/twilio-agent-connect-typescript#81):

1. create_observation() sent an unwrapped body. The Observations endpoint is a batch-create API expecting {"observations": [ { ... } ]}, but the method posted a single observation at the top level. It also only included occurredAt when the caller passed it. The body is now wrapped in an observations array, and occurredAt defaults to the current UTC time (ISO 8601) when the argument is None. The method stays public.

2. Removed internal post-conversation observation auto-creation. Per a product decision, TAC no longer mechanically creates observations after conversations end. Removed the observation dispatch branch in _process_operator_result, the _process_observation_event method, and its _parse_observations_content helper. An operator result matching the summary operator is processed; anything else skips. The summary path is fully intact.

3. Removed the now-unused observation_operator_sid config field. With observation auto-creation gone, observation_operator_sid on ConversationIntelligenceConfig was a write-only field that nothing reads. It is fully removed: the field declaration, the CONVERSATION_INTELLIGENCE_OBSERVATION_OPERATOR_SID read in from_env, and all doc/example references. Treated as non-breaking since Pydantic ignores unknown fields, so existing configs that still set it continue to construct.

Type of Change

• Bug fix
• Refactoring

Checklist

• Tests added/updated
• Documentation updated

SDK Parity

This is the Python SDK parity fix for twilio/twilio-agent-connect-typescript#81.

• Change is Python-specific (no TypeScript update needed)
• TypeScript SDK PR created: Fix createObservation() 400 and remove internal post-conversation observation auto-creation twilio-agent-connect-typescript#81

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes the Conversation Memory observation flow by aligning create_observation() with the batch-create API contract and removing post-conversation observation auto-creation from the intelligence event processor.

Changes:

• Wrap create_observation() request bodies in an observations array and default occurredAt when omitted.
• Remove observation operator parsing/dispatching from OperatorResultProcessor and adjust tests accordingly.
• Deprecate (accept-but-ignore) observation_operator_sid in config docs.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/test_profile_retrieval.py	Adds HTTP-level tests asserting create_observation() payload wrapping and default timestamp behavior.
tests/test_intelligence.py	Removes observation parsing/creation tests and updates processor tests to reflect observation auto-creation removal.
src/tac/intelligence/operator_result_processor.py	Removes observation parsing/dispatch path and updates docs/logic to only create summaries.
src/tac/core/config.py	Deprecates observation_operator_sid via description update while keeping field for compatibility.
src/tac/context/memory.py	Fixes create_observation() payload shape and defaults occurredAt to current UTC time.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

🟡 Not ready to approve

create_observation() and its new tests currently allow/send invalid occurredAt values (e.g., empty string), which can lead to guaranteed API request failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (3)

src/tac/context/memory.py:382

• create_observation() now always includes occurredAt, which means passing an empty/whitespace string will send an invalid ISO-8601 timestamp to the API. Since the docstring says occurred_at is ISO 8601, it should either default when blank or raise a ValueError before making the request.

        if occurred_at is None:
            occurred_at = datetime.now(timezone.utc).isoformat()
        observation["occurredAt"] = occurred_at

tests/test_profile_retrieval.py:724

• This test currently enforces that occurred_at="" is preserved and sent to the API, but that value is not a valid ISO-8601 timestamp and will likely cause a 4xx. If blank strings are treated as “not provided”, the assertion should expect a non-empty default timestamp (or the call should raise).

            await client.create_observation(
                profile_id="mem_profile_01canonical",
                content="Customer prefers email",
                occurred_at="",
            )

tests/test_profile_retrieval.py:703

• Docstring is now inaccurate if occurred_at is defaulted for blank/whitespace strings (recommended to avoid sending invalid timestamps). Update it to reflect the actual defaulting rule.

This issue also appears on line 720 of the same file.

        """Only default occurred_at when it is None, not for other falsy values."""

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The updated operator filtering logic can still validate/err on non-summary operator results before SID filtering, which risks failing whole webhook events that should be skipped under the new summary-only behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (3)

tests/test_intelligence.py:348

• This docstring still mentions observations, but the observation parsing tests were removed and this class now only tests summaries parsing.

    """Test content parsing for observations and summaries."""

src/tac/intelligence/operator_result_processor.py:316

• Operator SID filtering happens here, but _process_operator_result has already extracted profile IDs and generated content earlier in the method. That means non-summary operator results (SID mismatch) can still trigger hard failures (e.g., missing participants/content) and fail the whole webhook even though they should be skipped under the new summary-only behavior. Consider moving the summary_operator_sid/operator_id checks to the top of _process_operator_result so mismatched operators are skipped before any validation/parsing.

        # Determine event type by operator SID and process
        operator_id = operator_result.operator.id if operator_result.operator else None

        # Check if operator matches the configured summary SID.
        if self.config.summary_operator_sid is None:
            # No summary operator configured - nothing to process
            self.logger.debug("Skipping operator - summary operator SID not configured")
            return OperatorProcessingResult(
                success=True,
                skipped=True,
                skip_reason="Summary operator SID not configured",
            )

src/tac/core/config.py:45

• from_env() will treat an empty/whitespace CONVERSATION_INTELLIGENCE_SUMMARY_OPERATOR_SID as a configured (non-None) value. That can lead to confusing “Operator SID mismatch” skips instead of “not configured”, and it effectively configures an invalid SID. It’d be more robust to normalize blank values to None.

        return cls(
            configuration_id=configuration_id,
            summary_operator_sid=os.environ.get("CONVERSATION_INTELLIGENCE_SUMMARY_OPERATOR_SID"),
        )

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

There are a few concrete normalization/documentation issues (env whitespace handling, occurred_at trimming, and a misleading docstring example) that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (3)

src/tac/context/memory.py:382

• occurred_at is treated as blank/whitespace-only, but if a caller passes a valid timestamp with leading/trailing whitespace it will be sent untrimmed. Stripping in the non-blank case avoids hard-to-debug API rejections while preserving the provided value.

        if occurred_at is None or not occurred_at.strip():
            occurred_at = datetime.now(timezone.utc).isoformat()
        observation["occurredAt"] = occurred_at

src/tac/core/config.py:50

• ConversationIntelligenceConfig.from_env() treats an empty configuration ID as unset, but a whitespace-only value (e.g., " ") will currently be accepted and produce an invalid config. Since you already normalize blank operator SIDs, it's consistent to also strip/normalize CONVERSATION_INTELLIGENCE_CONFIGURATION_ID.

        configuration_id = os.environ.get("CONVERSATION_INTELLIGENCE_CONFIGURATION_ID")

        if not configuration_id:
            return None

src/tac/intelligence/operator_result_processor.py:148

• In the docstring example, the control-flow checks result.success before result.skipped, but skipped results are returned with success=True. As a result, a skipped event will print a misleading "Created 0 None(s)" and never reach the skipped branch; check skipped first in the example.

        conversation_memory_client = MemoryClient(...)
        config = ConversationIntelligenceConfig(
            configuration_id="GA...",
            summary_operator_sid="LY...",
        )
        processor = OperatorResultProcessor(conversation_memory_client, config)

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.