fix(logs): redact Customer Content from error/warning logs - #377
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughFailure errors and warnings across structured-output parsing, document expansion, query reformulation, and profile extraction now omit raw model-generated content while retaining length or value type metadata. ChangesLLM output redaction
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@reflexio/server/llm/_litellm_structured_output.py`:
- Around line 259-261: Update the StructuredOutputParseError construction in the
structured-output parsing flow to avoid interpolating the raw exception message
into the error text. Use only a fixed failure code or type(e).__name__, and if
exception chaining is needed, chain a sanitized exception so later error=%s
logging and LiteLLMClientError wrapping cannot expose customer content.
In `@reflexio/server/services/pre_retrieval/_document_expander.py`:
- Around line 156-161: Update the warning log in the document expansion
JSON-parse error path to report the raw model response length using output,
rather than the normalized text buffer. Keep the content omitted and preserve
the existing privacy-safe logging behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e4dd3a96-6bcf-4e6a-b165-f0d75f182f95
📒 Files selected for processing (4)
reflexio/server/llm/_litellm_structured_output.pyreflexio/server/services/pre_retrieval/_document_expander.pyreflexio/server/services/pre_retrieval/_query_reformulator.pyreflexio/server/services/profile/components/extractor.py
These log/exception sites interpolated model output or user/profile text into ERROR/WARNING messages, which flow to Sentry + CloudWatch via the logging bridge (before_send scrubs request bodies but not log/exception/breadcrumb content). Log lengths/types instead so no Customer Content reaches error monitoring: - _litellm_structured_output: drop the 200-char content snippet from the StructuredOutputParseError message (raw_content attribute kept for in-process repair; it is not serialized to logs). - profile extractor: log the type, not the raw profile dict. - query reformulator: log length/type, not the user query text. - document expander: log length, not the raw model output.
…review-loop) - structured output: use type(e).__name__ not str(e) in the parse-failure message (a json/pydantic error echoes the offending input), and let a deliberately-raised StructuredOutputParseError propagate unchanged so the content-free repair diagnostic (e.g. 'truncated') is preserved - profile extractor: log raw-profile COUNT, not the profile dicts (INFO records become Sentry breadcrumbs that before_send does not scrub) - document expander: report raw output length, not the normalized buffer
guangyu-reflexio
force-pushed
the
fix/redact-customer-content-logs
branch
from
July 22, 2026 14:08
4ed8d3e to
69ce7d1
Compare
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 27, 2026
A publish of 50 interactions returned 200 and stored 50 rows with content=''. No profiles were generated and nothing reported an error. Three defects combined to hide it. 1. InteractionData defaults every field and unknown keys were dropped with no trace, so a mis-keyed field yielded a *valid* interaction carrying nothing. 2. The precondition guard written to catch exactly this was dead code. UserActionType is a StrEnum whose NONE member is the truthy string "none", so `not interaction_data.user_action` was always False and the four-way "all empty" chain could never fire. Ten lines above, the same function already used the correct `!= UserActionType.NONE`. A test pinned the broken behaviour as correct, which is why it survived; that test is now inverted. 3. On the default async path the rejection is discarded -- add_user_interaction runs inside a BackgroundTask whose return value was dropped, after the caller already got 200 "queued". A success=False is not an exception, so it was not logged either. The per-interaction rules therefore live in a model_validator on PublishUserInteractionRequest, which runs during request parsing and so yields a 422 on both the sync and the background-task path. InteractionData.validation_error() holds all three rules (emptiness, user_action-needs-description, image-url-xor-encoding) in one place and precondition_checks delegates to it, so the layers cannot diverge and the two rules that previously existed only on the discarded path are now reportable. carries_content() counts every content-bearing field -- tools_used, shadow/expert content, citations, retrieved_learnings -- because a narrower list would reject legitimate tool-call-only and shadow-mode turns, and it strips text so " " is not content. Unknown keys are reported, not rejected. extra="forbid" was implemented first and reverted: both first-party plugins build their wire payload with a denylist, so every turn carries request-level bookkeeping such as user_id. Forbidding it raised, the plugin adapter swallowed the exception, and its publish watermark never advanced -- so the same batch retried forever and nothing was ever published, for every installed plugin, the moment the server deployed. That is strictly worse than the bug being fixed. Unknown keys are now captured, stripped so they cannot reach storage, and their NAMES (never values, per #377) returned in the response warnings[] and logged once server-side. Also: - openclaw plugin builds its wire dict from an allowlist of InteractionData fields instead of a three-key denylist, with a contract test pinning the allowlist to the real model and asserting every emitted turn validates. - The background rejection log withholds response.message, which on the storage path is an unbounded str(e) from a catch-all that Sentry would ingest as an unscrubbed event body. - AI_AGENT_INTEGRATION.md no longer tells integrators to hold the watermark on every failure -- that guidance is what turns a 4xx into a permanent wedge. - Fixes a route-test fixture that was itself posting user_message/ agent_message/interaction_type -- none of which are InteractionData fields -- and asserting 200 on the resulting empty interaction. BREAKING CHANGE: an interaction carrying nothing at all, a user_action without a user_action_description, or both interacted_image_url and image_encoding, now returns 422 instead of being accepted and silently dropped. Unknown fields are still accepted but are now reported in warnings[].
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 27, 2026
A publish of 50 interactions returned 200 and stored 50 rows with content=''. No profiles were generated and nothing reported an error. Three defects combined to hide it. 1. InteractionData defaults every field and unknown keys were dropped with no trace, so a mis-keyed field yielded a *valid* interaction carrying nothing. 2. The precondition guard written to catch exactly this was dead code. UserActionType is a StrEnum whose NONE member is the truthy string "none", so `not interaction_data.user_action` was always False and the four-way "all empty" chain could never fire. Ten lines above, the same function already used the correct `!= UserActionType.NONE`. A test pinned the broken behaviour as correct, which is why it survived; that test is now inverted. 3. On the default async path the rejection is discarded -- add_user_interaction runs inside a BackgroundTask whose return value was dropped, after the caller already got 200 "queued". A success=False is not an exception, so it was not logged either. The rules live in a model_validator on PublishUserInteractionRequest, which runs during request parsing and so applies on both the sync and the background-task path. InteractionData.shape_error() holds the contradictions (user_action-needs-description, image-url-xor-encoding) that previously existed only on the discarded path, and precondition_checks delegates to it so the layers cannot diverge. Two rules deliberately do NOT hard-fail, both because a stricter version was implemented, reproduced as catastrophic, and reverted: - Unknown keys are reported, not rejected. extra="forbid" broke every publish from both first-party plugins: they build their wire payload with a denylist, so each turn carries request-level bookkeeping such as user_id. The adapter swallows the resulting error and never advances its publish watermark, so the same batch retries forever and nothing is published -- for every installed plugin, the moment the server deploys. Unknown keys are now captured, stripped so they cannot reach storage, and their NAMES (never values, per #377) reported in warnings[]. Nested payload models capture the same way, which surfaces ToolUsed.status -- a field both plugins send on every tool call and that was being dropped silently. - An individual empty interaction is skipped, not fatal. Both plugins append an empty Assistant placeholder unconditionally, so failing the batch wedged them exactly as above -- rejecting the real user turn beside the placeholder. A batch where *every* interaction is empty is still a 422, and that is precisely the incident (50 of 50 rows). carries_content() counts every content-bearing field -- tools_used, shadow/expert content, citations, retrieved_learnings -- because a narrower list would reject legitimate tool-call-only and shadow-mode turns, and it strips text so " " is not content. Also: - Warning volume is bounded: unknown key names are caller-controlled and uncapped, and 2000 long keys produced a 411 KB warning echoed into both the response and a single log record. Now capped at 5 truncated names plus a count. - Request-level keys callers duplicate per-interaction (user_id, session_id) are stripped but not warned about, so a correct claude-smart batch emits 0 warnings rather than one per turn. - Both background-task log lines withhold their reason string. On the storage path that is an unbounded str(e) from a catch-all, and Sentry ingests ERROR records as event bodies without scrubbing them. - openclaw plugin builds its wire dict from an allowlist of InteractionData fields, with a contract test pinning the allowlist to the real model and asserting every emitted turn validates. - AI_AGENT_INTEGRATION.md no longer tells integrators to hold the watermark on every failure -- that guidance is what turns a 4xx into a permanent wedge. It now splits retryable from non-retryable. - Fixes a route-test fixture that was itself posting user_message/ agent_message/interaction_type -- none of which are InteractionData fields -- and asserting 200 on the resulting empty interaction. BREAKING CHANGE: a publish where every interaction is empty, an interaction whose user_action has no user_action_description, or one setting both interacted_image_url and image_encoding, now returns 422 instead of being accepted and silently dropped. Unknown fields and individually-empty interactions are still accepted, and are now reported in the response's warnings[].
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 27, 2026
A publish of 50 interactions returned 200 and stored 50 rows with content=''. No profiles were generated and nothing reported an error. Three defects combined to hide it. 1. InteractionData defaults every field and unknown keys were dropped with no trace, so a mis-keyed field yielded a *valid* interaction carrying nothing. 2. The precondition guard written to catch exactly this was dead code. UserActionType is a StrEnum whose NONE member is the truthy string "none", so `not interaction_data.user_action` was always False and the four-way "all empty" chain could never fire. Ten lines above, the same function already used the correct `!= UserActionType.NONE`. A test pinned the broken behaviour as correct, which is why it survived; that test is now inverted. 3. On the default async path the rejection is discarded -- add_user_interaction runs inside a BackgroundTask whose return value was dropped, after the caller already got 200 "queued". A success=False is not an exception, so it was not logged either. The rules live in a model_validator on PublishUserInteractionRequest, which runs during request parsing and so applies on both the sync and the background-task path. InteractionData.shape_error() holds the contradictions (user_action-needs-description, image-url-xor-encoding) that previously existed only on the discarded path, and precondition_checks delegates to it so the layers cannot diverge. Two rules deliberately do NOT hard-fail. Stricter versions of both were implemented, reproduced as catastrophic, and reverted -- in each case because the first-party plugins build their wire payload with a denylist and buffer an empty Assistant placeholder unconditionally, while their adapters swallow the error and never advance the publish watermark. The same batch then retries forever and nothing is ever published, for every installed plugin, the moment the server deploys: - Unknown keys are reported, not rejected. They are captured, stripped so they cannot reach storage, and their NAMES (never values, per #377) returned in warnings[]. Nested payload models capture the same way, which surfaces ToolUsed.status -- a field both plugins send on every tool call that was being dropped silently. - An individual empty interaction is skipped, not fatal. A batch where *every* interaction is empty is still a 422, and that is precisely the incident (50 of 50 rows). Warnings are built against the caller's ORIGINAL list, before empty rows are filtered out. Computing them afterwards defeated the feature in its primary case: a mis-keyed content yields an empty interaction, so the row carrying the typo was exactly the row removed and its warning vanished, leaving the caller "skipped 1 empty interaction" with no idea which field was wrong -- and every surviving index was renumbered. Warning output is bounded on three axes (per-name length, names per interaction, total entries) and control characters are stripped, because the names are caller-controlled and reach both the response body and a shared multi-tenant log stream. Unbounded, 1000 interactions produced a ~350 KB warning and one enormous log record; now ~2.5 KB. carries_content() counts every content-bearing field -- tools_used, shadow/expert content, citations, retrieved_learnings -- because a narrower list would reject legitimate tool-call-only and shadow-mode turns, and it strips text so " " is not content. Also: - Request-level keys callers duplicate per-interaction (user_id, session_id) are stripped but not warned about, so a correct claude-smart batch emits 0 warnings rather than one per turn. - Both background-task log lines withhold their reason string. On the storage path that is an unbounded str(e) from a catch-all, and Sentry ingests ERROR records as event bodies without scrubbing them. - openclaw plugin builds its wire dict from an allowlist of InteractionData fields. The drift guard lives in tests/, not the plugin's own suite: testpaths excludes that directory and no workflow runs it, so an assertion there would never have executed. - AI_AGENT_INTEGRATION.md replaces "hold the watermark on every failure" with a per-status table -- hold on 5xx/timeout/408/429, hold and escalate on 401/403, advance only on 400/422 -- since a blanket hold is what turns a rejection into a permanent wedge. - Fixes a route-test fixture that was itself posting user_message/ agent_message/interaction_type -- none of which are InteractionData fields -- and asserting 200 on the resulting empty interaction. BREAKING CHANGE: a publish where every interaction is empty, an interaction whose user_action has no user_action_description, or one setting both interacted_image_url and image_encoding, now returns 422 instead of being accepted and silently dropped. Unknown fields and individually-empty interactions are still accepted, and are now reported in the response's warnings[].
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 27, 2026
A publish of 50 interactions returned 200 and stored 50 rows with content=''. No profiles were generated and nothing reported an error. Three defects combined to hide it. 1. InteractionData defaults every field and unknown keys were dropped with no trace, so a mis-keyed field yielded a *valid* interaction carrying nothing. 2. The precondition guard written to catch exactly this was dead code. UserActionType is a StrEnum whose NONE member is the truthy string "none", so `not interaction_data.user_action` was always False and the four-way "all empty" chain could never fire. Ten lines above, the same function already used the correct `!= UserActionType.NONE`. A test pinned the broken behaviour as correct, which is why it survived; that test is now inverted. 3. On the default async path the rejection is discarded -- add_user_interaction runs inside a BackgroundTask whose return value was dropped, after the caller already got 200 "queued". A success=False is not an exception, so it was not logged either. The rules live in a model_validator on PublishUserInteractionRequest, which runs during request parsing and so applies on both the sync and the background-task path. InteractionData.shape_error() holds the contradictions (user_action-needs-description, image-url-xor-encoding) that previously existed only on the discarded path, and precondition_checks delegates to it so the layers cannot diverge. Two rules deliberately do NOT hard-fail. Stricter versions of both were implemented, reproduced as catastrophic, and reverted -- in each case because the first-party plugins build their wire payload with a denylist and buffer an empty Assistant placeholder unconditionally, while their adapters swallow the error and never advance the publish watermark. The same batch then retries forever and nothing is ever published, for every installed plugin, the moment the server deploys: - Unknown keys are reported, not rejected. They are captured, stripped so they cannot reach storage, and their NAMES (never values, per #377) returned in warnings[]. Nested payload models capture the same way, which surfaces ToolUsed.status -- a field both plugins send on every tool call that was being dropped silently. - An individual empty interaction is skipped, not fatal. A batch where *every* interaction is empty is still a 422, and that is precisely the incident (50 of 50 rows). Warnings are built against the caller's ORIGINAL list, before empty rows are filtered out. Computing them afterwards defeated the feature in its primary case: a mis-keyed content yields an empty interaction, so the row carrying the typo was exactly the row removed and its warning vanished, leaving the caller "skipped 1 empty interaction" with no idea which field was wrong -- and every surviving index was renumbered. The client merges its own locally-detected warnings into the response. publish_interaction builds InteractionData before model_dump(), so the unknown keys are already stripped and the server never sees them; without the merge, unrecognised fields were reported over raw HTTP but invisible through the SDK, which is the primary integration path. Warning output is bounded on three axes (per-name length, names per interaction, total entries) and control characters are stripped, because the names are caller-controlled and reach both the response body and a shared multi-tenant log stream. Unbounded, 1000 interactions produced a ~350 KB warning and one enormous log record; now ~2.5 KB. carries_content() counts every content-bearing field -- tools_used, shadow/expert content, citations, retrieved_learnings -- because a narrower list would reject legitimate tool-call-only and shadow-mode turns, and it strips text so " " is not content. Also: - Request-level keys callers duplicate per-interaction (user_id, session_id) are stripped but not warned about, so a correct claude-smart batch emits 0 warnings rather than one per turn. - Both background-task log lines withhold their reason string. On the storage path that is an unbounded str(e) from a catch-all, and Sentry ingests ERROR records as event bodies without scrubbing them. - openclaw plugin builds its wire dict from an allowlist of InteractionData fields. The drift guard lives in tests/, not the plugin's own suite: testpaths excludes that directory and no workflow runs it, so an assertion there would never have executed. - AI_AGENT_INTEGRATION.md replaces "hold the watermark on every failure" with a per-status table -- hold on 5xx/timeout/408/429, hold and escalate on 401/403, advance only on 400/422 -- since a blanket hold is what turns a rejection into a permanent wedge. - Fixes a route-test fixture that was itself posting user_message/ agent_message/interaction_type -- none of which are InteractionData fields -- and asserting 200 on the resulting empty interaction. BREAKING CHANGE: a publish where every interaction is empty, an interaction whose user_action has no user_action_description, or one setting both interacted_image_url and image_encoding, now returns 422 instead of being accepted and silently dropped. Unknown fields and individually-empty interactions are still accepted, and are now reported in the response's warnings[].
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 27, 2026
A publish of 50 interactions returned 200 and stored 50 rows with content=''. No profiles were generated and nothing reported an error. Three defects combined to hide it. 1. InteractionData defaults every field and unknown keys were dropped with no trace, so a mis-keyed field yielded a *valid* interaction carrying nothing. 2. The precondition guard written to catch exactly this was dead code. UserActionType is a StrEnum whose NONE member is the truthy string "none", so `not interaction_data.user_action` was always False and the four-way "all empty" chain could never fire. Ten lines above, the same function already used the correct `!= UserActionType.NONE`. A test pinned the broken behaviour as correct, which is why it survived; that test is now inverted. 3. On the default async path the rejection is discarded -- add_user_interaction runs inside a BackgroundTask whose return value was dropped, after the caller already got 200 "queued". A success=False is not an exception, so it was not logged either. The rules live in a model_validator on PublishUserInteractionRequest, which runs during request parsing and so applies on both the sync and the background-task path. InteractionData.shape_error() holds the contradictions (user_action-needs-description, image-url-xor-encoding) that previously existed only on the discarded path, and precondition_checks delegates to it so the layers cannot diverge. Two rules deliberately do NOT hard-fail. Stricter versions of both were implemented, reproduced as catastrophic, and reverted -- in each case because the first-party plugins build their wire payload with a denylist and buffer an empty Assistant placeholder unconditionally, while their adapters swallow the error and never advance the publish watermark. The same batch then retries forever and nothing is ever published, for every installed plugin, the moment the server deploys: - Unknown keys are reported, not rejected. They are captured, stripped so they cannot reach storage, and their NAMES (never values, per #377) returned in warnings[]. Nested payload models capture the same way, which surfaces ToolUsed.status -- a field both plugins send on every tool call that was being dropped silently. - An individual empty interaction is skipped, not fatal. A batch where *every* interaction is empty is still a 422, and that is precisely the incident (50 of 50 rows). Warnings are built against the caller's ORIGINAL list, before empty rows are filtered out. Computing them afterwards defeated the feature in its primary case: a mis-keyed content yields an empty interaction, so the row carrying the typo was exactly the row removed and its warning vanished, leaving the caller "skipped 1 empty interaction" with no idea which field was wrong -- and every surviving index was renumbered. The client merges its own locally-detected warnings into the response. publish_interaction builds InteractionData before model_dump(), so the unknown keys are already stripped and the server never sees them; without the merge, unrecognised fields were reported over raw HTTP but invisible through the SDK, which is the primary integration path. Warning output is bounded on three axes (per-name length, names per interaction, total entries) and control characters are stripped, because the names are caller-controlled and reach both the response body and a shared multi-tenant log stream. Unbounded, 1000 interactions produced a ~350 KB warning and one enormous log record; now ~2.5 KB. carries_content() counts every content-bearing field -- tools_used, shadow/expert content, citations, retrieved_learnings -- because a narrower list would reject legitimate tool-call-only and shadow-mode turns, and it strips text so " " is not content. Also: - Request-level keys callers duplicate per-interaction (user_id, session_id) are stripped but not warned about, so a correct claude-smart batch emits 0 warnings rather than one per turn. - Both background-task log lines withhold their reason string. On the storage path that is an unbounded str(e) from a catch-all, and Sentry ingests ERROR records as event bodies without scrubbing them. - openclaw plugin builds its wire dict from an allowlist of InteractionData fields. The drift guard lives in tests/, not the plugin's own suite: testpaths excludes that directory and no workflow runs it, so an assertion there would never have executed. - AI_AGENT_INTEGRATION.md replaces "hold the watermark on every failure" with a per-status table -- hold on 5xx/timeout/408/429, hold and escalate on 401/403, advance only on 400/422 -- since a blanket hold is what turns a rejection into a permanent wedge. - Fixes a route-test fixture that was itself posting user_message/ agent_message/interaction_type -- none of which are InteractionData fields -- and asserting 200 on the resulting empty interaction. - ToolUsed gains a declared `status` field. Both plugins already send it ("success"/"error", derived from the tool response); it was being discarded, losing real signal, and once nested capture began reporting unknown keys it became the single largest warning source -- one per tool-bearing turn. Declaring it recovers the data and removes the noise. - The skipped-empty summary is appended AFTER the entry cap, so the cap can never drop it. Capping the combined list swallowed "N interactions were dropped" whenever there were >= 20 unknown-field warnings, which is the one thing the caller most needs to know about such a batch. - The openclaw adapter captures publish_interaction's return value and logs any warnings. It previously discarded the result, so the integration that motivated this work could not see its own dropped fields. - The SDK-side test moved to tests/client/, its mirror location, using the real ReflexioClient constructor instead of __new__ plus stubs; plus a test pinning the premise the client-side merge rests on (the wire payload carries no unknown keys, so warnings cannot double). BREAKING CHANGE: a publish where every interaction is empty, an interaction whose user_action has no user_action_description, or one setting both interacted_image_url and image_encoding, now returns 422 instead of being accepted and silently dropped. Unknown fields and individually-empty interactions are still accepted, and are now reported in the response's warnings[].
guangyu-reflexio
added a commit
that referenced
this pull request
Jul 27, 2026
A publish of 50 interactions returned 200 and stored 50 rows with content=''. No profiles were generated and nothing reported an error. Three defects combined to hide it. 1. InteractionData defaults every field and unknown keys were dropped with no trace, so a mis-keyed field yielded a *valid* interaction carrying nothing. 2. The precondition guard written to catch exactly this was dead code. UserActionType is a StrEnum whose NONE member is the truthy string "none", so `not interaction_data.user_action` was always False and the four-way "all empty" chain could never fire. Ten lines above, the same function already used the correct `!= UserActionType.NONE`. A test pinned the broken behaviour as correct, which is why it survived; that test is now inverted. 3. On the default async path the rejection is discarded -- add_user_interaction runs inside a BackgroundTask whose return value was dropped, after the caller already got 200 "queued". A success=False is not an exception, so it was not logged either. The rules live in a model_validator on PublishUserInteractionRequest, which runs during request parsing and so applies on both the sync and the background-task path. InteractionData.shape_error() holds the contradictions (user_action-needs-description, image-url-xor-encoding) that previously existed only on the discarded path, and precondition_checks delegates to it so the layers cannot diverge. Two rules deliberately do NOT hard-fail. Stricter versions of both were implemented, reproduced as catastrophic, and reverted -- in each case because the first-party plugins build their wire payload with a denylist and buffer an empty Assistant placeholder unconditionally, while their adapters swallow the error and never advance the publish watermark. The same batch then retries forever and nothing is ever published, for every installed plugin, the moment the server deploys: - Unknown keys are reported, not rejected. They are captured, stripped so they cannot reach storage, and their NAMES (never values, per #377) returned in warnings[]. Nested payload models capture the same way, which surfaces ToolUsed.status -- a field both plugins send on every tool call that was being dropped silently. - An individual empty interaction is skipped, not fatal. A batch where *every* interaction is empty is still a 422, and that is precisely the incident (50 of 50 rows). Warnings are built against the caller's ORIGINAL list, before empty rows are filtered out. Computing them afterwards defeated the feature in its primary case: a mis-keyed content yields an empty interaction, so the row carrying the typo was exactly the row removed and its warning vanished, leaving the caller "skipped 1 empty interaction" with no idea which field was wrong -- and every surviving index was renumbered. The client merges its own locally-detected warnings into the response. publish_interaction builds InteractionData before model_dump(), so the unknown keys are already stripped and the server never sees them; without the merge, unrecognised fields were reported over raw HTTP but invisible through the SDK, which is the primary integration path. Warning output is bounded on three axes (per-name length, names per interaction, total entries) and control characters are stripped, because the names are caller-controlled and reach both the response body and a shared multi-tenant log stream. Unbounded, 1000 interactions produced a ~350 KB warning and one enormous log record; now ~2.5 KB. carries_content() counts every content-bearing field -- tools_used, shadow/expert content, citations, retrieved_learnings -- because a narrower list would reject legitimate tool-call-only and shadow-mode turns, and it strips text so " " is not content. Also: - Request-level keys callers duplicate per-interaction (user_id, session_id) are stripped but not warned about, so a correct claude-smart batch emits 0 warnings rather than one per turn. - Both background-task log lines withhold their reason string. On the storage path that is an unbounded str(e) from a catch-all, and Sentry ingests ERROR records as event bodies without scrubbing them. - openclaw plugin builds its wire dict from an allowlist of InteractionData fields. The drift guard lives in tests/, not the plugin's own suite: testpaths excludes that directory and no workflow runs it, so an assertion there would never have executed. - AI_AGENT_INTEGRATION.md replaces "hold the watermark on every failure" with a per-status table -- hold on 5xx/timeout/408/429, hold and escalate on 401/403, advance only on 400/422 -- since a blanket hold is what turns a rejection into a permanent wedge. - Fixes a route-test fixture that was itself posting user_message/ agent_message/interaction_type -- none of which are InteractionData fields -- and asserting 200 on the resulting empty interaction. - ToolUsed gains a declared `status` field. Both plugins already send it ("success"/"error", derived from the tool response); it was being discarded, losing real signal, and once nested capture began reporting unknown keys it became the single largest warning source -- one per tool-bearing turn. Declaring it recovers the data and removes the noise. - The skipped-empty summary is appended AFTER the entry cap, so the cap can never drop it. Capping the combined list swallowed "N interactions were dropped" whenever there were >= 20 unknown-field warnings, which is the one thing the caller most needs to know about such a batch. - The openclaw adapter captures publish_interaction's return value and logs any warnings. It previously discarded the result, so the integration that motivated this work could not see its own dropped fields. - The SDK-side test moved to tests/client/, its mirror location, using the real ReflexioClient constructor instead of __new__ plus stubs; plus a test pinning the premise the client-side merge rests on (the wire payload carries no unknown keys, so warnings cannot double). - ToolUsed gains a declared `status` field, coerced rather than validated. Both plugins already send it ("success"/"error"); it was being discarded, and once nested capture began reporting unknown keys it was the largest single warning source. Declaring it strictly turned five previously- harmless values (int, None, bool, dict, >100 chars) into a 422 for the WHOLE batch, which the plugin adapters swallow without advancing their watermark -- reintroducing the exact stall that made extra="forbid" unacceptable. A mode="before" validator coerces and truncates instead. - request_id is sanitised before it reaches a log line. It is a NonEmptyStr with no length cap and no character restrictions, so a newline in it could forge a line in a shared multi-tenant log stream that Sentry ingests -- the same hazard as the unknown field names, on the value beside them. - The openclaw adapter's warning diagnostics sit outside the try that decides the publish result, and cannot raise. Inside it, a failure while merely formatting a warning would be read as a publish failure and stall the watermark on an already-successful publish. - cap_warning_list returns a copy; the caller appends to it. BREAKING CHANGE: a publish where every interaction is empty, an interaction whose user_action has no user_action_description, or one setting both interacted_image_url and image_encoding, now returns 422 instead of being accepted and silently dropped. Unknown fields and individually-empty interactions are still accepted, and are now reported in the response's warnings[].
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Redacts Customer Content (LLM output snippets, profile text, user queries) from ERROR/WARNING log and breadcrumb sites so it does not reach error monitoring (Sentry) or CloudWatch.
Why
Supports the reflexio.ai managed-platform Privacy Policy representation that Customer Content is not sent to analytics or error-monitoring providers. Before this change, a structured-output parse error logged a ~200-char LLM output snippet at ERROR, and profile text / user queries were attached as breadcrumbs — which made Sentry/CloudWatch a Customer-Data recipient.
Changes
_litellm_structured_output.py: drop the content snippet from the parse-error log message (retainraw_contentas a structured attribute only)extractor.py,_query_reformulator.py,_document_expander.py: log length/type instead of the content textScope
Log-message redaction only — no behavior change to extraction or generation. Paired with the reflexio-enterprise legal-docs PR, which relies on this for its "no Customer Content in logs" representation.
Summary by CodeRabbit